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

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

[JiangJieHua]

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.

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

A TCP packet whose transport header cannot be read now logs:

  before: mark=0x0
  after:  mark=0x0 saddr=10.20.0.2 daddr=10.20.0.1 proto=6 sport=? dport=?

Fixes: f19590b07cb6 ("audit: add audit_log_nf_skb helper function")
Fixes: 15b0c43aa621 ("audit: include source and destination ports to NETFILTER_PKT")
Signed-off-by: JiangJieHua <jiangjiehua1@huawei.com>
---
 include/linux/audit.h |  12 ++---
 kernel/audit.c        | 114 ++++++++++++++++++++++--------------------
 2 files changed, 65 insertions(+), 61 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 767e349d4834..8263257a010e 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -197,8 +197,8 @@ extern int audit_log_subj_ctx(struct audit_buffer *ab, struct lsm_prop *prop);
 extern int audit_log_obj_ctx(struct audit_buffer *ab, struct lsm_prop *prop);
 extern int audit_log_task_context(struct audit_buffer *ab);
 extern void audit_log_task_info(struct audit_buffer *ab);
-extern int audit_log_nf_skb(struct audit_buffer *ab,
-			    const struct sk_buff *skb, u8 nfproto);
+extern void audit_log_nf_skb(struct audit_buffer *ab,
+			     const struct sk_buff *skb, u8 nfproto);
 
 extern int		    audit_update_lsm_rules(void);
 
@@ -276,11 +276,9 @@ static inline int audit_log_task_context(struct audit_buffer *ab)
 static inline void audit_log_task_info(struct audit_buffer *ab)
 { }
 
-static inline int audit_log_nf_skb(struct audit_buffer *ab,
-				   const struct sk_buff *skb, u8 nfproto)
-{
-	return 0;
-}
+static inline void audit_log_nf_skb(struct audit_buffer *ab,
+				    const struct sk_buff *skb, u8 nfproto)
+{ }
 
 static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 {
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;
+
+		audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
+				 &ih->saddr, &ih->daddr, ih->protocol);
 
 		switch (ih->protocol) {
 		case IPPROTO_TCP: {
 			struct tcphdr _tcph;
 			const struct tcphdr *th;
 
-			th = skb_header_pointer(skb, skb_transport_offset(skb),
-						sizeof(_tcph), &_tcph);
-			if (!th)
-				return -ENOMEM;
-
-			audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu sport=%hu dport=%hu",
-					 &ih->saddr, &ih->daddr, ih->protocol,
+			th = skb_header_pointer(skb, thoff, sizeof(_tcph),
+						&_tcph);
+			if (frag || !th)
+				goto unknown_ports;
+			audit_log_format(ab, " sport=%hu dport=%hu",
 					 ntohs(th->source), ntohs(th->dest));
 			break;
 		}
@@ -2558,13 +2566,11 @@ int audit_log_nf_skb(struct audit_buffer *ab,
 			struct udphdr _udph;
 			const struct udphdr *uh;
 
-			uh = skb_header_pointer(skb, skb_transport_offset(skb),
-						sizeof(_udph), &_udph);
-			if (!uh)
-				return -ENOMEM;
-
-			audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu sport=%hu dport=%hu",
-					 &ih->saddr, &ih->daddr, ih->protocol,
+			uh = skb_header_pointer(skb, thoff, sizeof(_udph),
+						&_udph);
+			if (frag || !uh)
+				goto unknown_ports;
+			audit_log_format(ab, " sport=%hu dport=%hu",
 					 ntohs(uh->source), ntohs(uh->dest));
 			break;
 		}
@@ -2572,19 +2578,14 @@ int audit_log_nf_skb(struct audit_buffer *ab,
 			struct sctphdr _sctph;
 			const struct sctphdr *sh;
 
-			sh = skb_header_pointer(skb, skb_transport_offset(skb),
-						sizeof(_sctph), &_sctph);
-			if (!sh)
-				return -ENOMEM;
-
-			audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu sport=%hu dport=%hu",
-					 &ih->saddr, &ih->daddr, ih->protocol,
+			sh = skb_header_pointer(skb, thoff, sizeof(_sctph),
+						&_sctph);
+			if (frag || !sh)
+				goto unknown_ports;
+			audit_log_format(ab, " sport=%hu dport=%hu",
 					 ntohs(sh->source), ntohs(sh->dest));
 			break;
 		}
-		default:
-			audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
-					 &ih->saddr, &ih->daddr, ih->protocol);
 		}
 
 		break;
@@ -2594,28 +2595,36 @@ int audit_log_nf_skb(struct audit_buffer *ab,
 		const struct ipv6hdr *ih;
 		u8 nexthdr;
 		__be16 frag_off;
+		int thoff;
 
 		ih = skb_header_pointer(skb, skb_network_offset(skb),
 					sizeof(iph), &iph);
 		if (!ih)
-			return -ENOMEM;
+			goto unknown_proto;
+		if (ih->version != 6)
+			goto unknown_proto;
 
 		nexthdr = ih->nexthdr;
-		ipv6_skip_exthdr(skb, skb_network_offset(skb) + sizeof(iph),
-				 &nexthdr, &frag_off);
+		thoff = ipv6_skip_exthdr(skb,
+					 skb_network_offset(skb) + sizeof(iph),
+					 &nexthdr, &frag_off);
+
+		audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu",
+				 &ih->saddr, &ih->daddr, nexthdr);
+
+		if (thoff < 0)
+			break;
 
 		switch (nexthdr) {
 		case IPPROTO_TCP: {
 			struct tcphdr _tcph;
 			const struct tcphdr *th;
 
-			th = skb_header_pointer(skb, skb_transport_offset(skb),
-						sizeof(_tcph), &_tcph);
+			th = skb_header_pointer(skb, thoff, sizeof(_tcph),
+						&_tcph);
 			if (!th)
-				return -ENOMEM;
-
-			audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu sport=%hu dport=%hu",
-					 &ih->saddr, &ih->daddr, nexthdr,
+				goto unknown_ports;
+			audit_log_format(ab, " sport=%hu dport=%hu",
 					 ntohs(th->source), ntohs(th->dest));
 			break;
 		}
@@ -2624,13 +2633,11 @@ int audit_log_nf_skb(struct audit_buffer *ab,
 			struct udphdr _udph;
 			const struct udphdr *uh;
 
-			uh = skb_header_pointer(skb, skb_transport_offset(skb),
-						sizeof(_udph), &_udph);
+			uh = skb_header_pointer(skb, thoff, sizeof(_udph),
+						&_udph);
 			if (!uh)
-				return -ENOMEM;
-
-			audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu sport=%hu dport=%hu",
-					 &ih->saddr, &ih->daddr, nexthdr,
+				goto unknown_ports;
+			audit_log_format(ab, " sport=%hu dport=%hu",
 					 ntohs(uh->source), ntohs(uh->dest));
 			break;
 		}
@@ -2638,19 +2645,14 @@ int audit_log_nf_skb(struct audit_buffer *ab,
 			struct sctphdr _sctph;
 			const struct sctphdr *sh;
 
-			sh = skb_header_pointer(skb, skb_transport_offset(skb),
-						sizeof(_sctph), &_sctph);
+			sh = skb_header_pointer(skb, thoff, sizeof(_sctph),
+						&_sctph);
 			if (!sh)
-				return -ENOMEM;
-
-			audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu sport=%hu dport=%hu",
-					 &ih->saddr, &ih->daddr, nexthdr,
+				goto unknown_ports;
+			audit_log_format(ab, " sport=%hu dport=%hu",
 					 ntohs(sh->source), ntohs(sh->dest));
 			break;
 		}
-		default:
-			audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu",
-					 &ih->saddr, &ih->daddr, nexthdr);
 		}
 
 		break;
@@ -2659,11 +2661,15 @@ int audit_log_nf_skb(struct audit_buffer *ab,
 		goto unknown_proto;
 	}
 
-	return 0;
+	return;
+
+unknown_ports:
+	audit_log_format(ab, " sport=? dport=?");
+	return;
 
 unknown_proto:
-	audit_log_format(ab, " saddr=? daddr=? proto=?");
-	return -EPFNOSUPPORT;
+	audit_log_format(ab, " saddr=? daddr=? proto=-1");
+	return;
 }
 EXPORT_SYMBOL(audit_log_nf_skb);
 
-- 
2.33.8

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?

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > +			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?

Bridge doesn't check/validate ip headers.

I agree it would be best to have a central helper for this, this
copypastry has gotten out of hand.