From: Bradley Morgan <bradley.morgan@linux.dev>
To: jiangjiehua1@huawei.com
Cc: audit@vger.kernel.org, coreteam@netfilter.org, eparis@redhat.com,
fw@strlen.de, linux-kernel@vger.kernel.org,
netfilter-devel@vger.kernel.org, pablo@netfilter.org,
paul@paul-moore.com, rrobaina@redhat.com,
zhangchangzhong@huawei.com
Subject: Re: [PATCH] audit: fix incomplete NETFILTER_PKT records and return void
Date: Tue, 10 Nov 2026 15:51:58 +0000 [thread overview]
Message-ID: <F79DD66E-99AE-44E0-87A3-E7B2FE704290@linux.dev> (raw)
In-Reply-To: <20261110122939.45373-1-jiangjiehua1@huawei.com>
On 10 November 2026 12:29:39 GMT, JiangJieHua <jiangjiehua1@huawei.com>
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.
>
>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")
This code looks very, very confusing. :(
>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);
>
>
--- Thanks!
prev parent reply other threads:[~2026-11-10 15:52 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-10 12:29 JiangJieHua
2026-11-10 13:10 ` Pablo Neira Ayuso
2026-11-10 13:39 ` Florian Westphal
2026-11-10 15:51 ` Bradley Morgan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=F79DD66E-99AE-44E0-87A3-E7B2FE704290@linux.dev \
--to=bradley.morgan@linux.dev \
--cc=audit@vger.kernel.org \
--cc=coreteam@netfilter.org \
--cc=eparis@redhat.com \
--cc=fw@strlen.de \
--cc=jiangjiehua1@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
--cc=paul@paul-moore.com \
--cc=rrobaina@redhat.com \
--cc=zhangchangzhong@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®