Re: [PATCH] audit: fix incomplete NETFILTER_PKT records and return void

[Pablo Neira Ayuso]

Hi,

On Tue, Nov 10, 2026 at 08:29:39PM +0800, JiangJieHua wrote:
> Commit f19590b07cb6 ("audit: add audit_log_nf_skb helper function") and
> commit 15b0c43aa621 ("audit: include source and destination ports to
> NETFILTER_PKT") introduced audit_log_nf_skb() and its transport header
> parsing.

I guess conntrack is not enough in your scenario. How does your
ruleset look like?

More comments below.

> The helper returned early with -ENOMEM when skb_header_pointer() failed for
> the network header, and the later port parsing did the same for the
> transport header.  Such an early return skips the remaining fields, so the
> NETFILTER_PKT record is emitted with only "mark=" and without the address
> and protocol fields.
> 
> Log the address and protocol once the network header is available, and
> route the error paths through placeholder logging: a missing network header
> logs "saddr=? daddr=? proto=-1", as the pre-refactoring callers did, and a
> transport header that cannot be read where one is expected logs
> "sport=? dport=?".  The same "proto=-1" placeholder is used for an
> unknown bridge ethertype and an unknown nfproto, which previously printed
> "proto=?".
> 
> Validate the IP version and the IPv4 header length, since bridge packets
> can reach the audit hook without the receive path validating them.  Locate
> the transport header from the network header instead of relying on
> skb_transport_offset(), which is not guaranteed to point at the transport
> header.  For IPv4, a non-first fragment or an unreadable transport header
> logs unknown ports; protocols without ports are unaffected.  For IPv6, a
> non-first fragment leaves proto at IPPROTO_FRAGMENT (44) and a negative
> offset from ipv6_skip_exthdr() means the extension header chain cannot be
> parsed; in both cases the transport header is not available, so keep the
> addresses and protocol and do not log ports.
> 
> Both callers ignore the return value (net/netfilter/nft_log.c and
> net/netfilter/xt_AUDIT.c), so make the helper return void instead of
> inventing error codes for a value nobody consumes.
> 
> A packet whose network header cannot be read now logs:
> 
>   before: mark=0x0
>   after:  mark=0x0 saddr=? daddr=? proto=-1
[...]
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 631d6d4a23cf..c5997077a46b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2511,8 +2511,8 @@ void audit_log_path_denied(int type, const char *operation)
>  	audit_log_end(ab);
>  }
>  
> -int audit_log_nf_skb(struct audit_buffer *ab,
> -		     const struct sk_buff *skb, u8 nfproto)
> +void audit_log_nf_skb(struct audit_buffer *ab,
> +		      const struct sk_buff *skb, u8 nfproto)
>  {
>  	/* find the IP protocol in the case of NFPROTO_BRIDGE */
>  	if (nfproto == NFPROTO_BRIDGE) {
> @@ -2532,24 +2532,32 @@ int audit_log_nf_skb(struct audit_buffer *ab,
>  	case NFPROTO_IPV4: {
>  		struct iphdr iph;
>  		const struct iphdr *ih;
> +		int thoff;
> +		bool frag;
>  
>  		ih = skb_header_pointer(skb, skb_network_offset(skb),
>  					sizeof(iph), &iph);
>  		if (!ih)
> -			return -ENOMEM;
> +			goto unknown_proto;
> +		if (ih->version != 4 || ih->ihl < 5)
> +			goto unknown_proto;
> +
> +		thoff = skb_network_offset(skb) + ih->ihl * 4;
> +		frag = ntohs(ih->frag_off) & IP_OFFSET;

Are we going to add all these validations everywhere in the code?

So we will have to validate that the IPv4 header is well-formed over
and over again?