| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAABTaaCVsFOmouRZED_DTMPy_EimSAsercz=8A3RLTUYnpvf_A@mail.gmail.com>
Date: Mon, 10 Nov 2025 09:16:32 -0300
From: Ricardo Robaina <rrobaina@...hat.com>
To: Paul Moore <paul@...l-moore.com>
Cc: audit@...r.kernel.org, linux-kernel@...r.kernel.org,
netfilter-devel@...r.kernel.org, coreteam@...filter.org, eparis@...hat.com,
fw@...len.de, pablo@...filter.org, kadlec@...filter.org
Subject: Re: [PATCH v5 1/2] audit: add audit_log_packet_ip4 and
audit_log_packet_ip6 helper functions
On Fri, Nov 7, 2025 at 7:46 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Nov 6, 2025 Ricardo Robaina <rrobaina@...hat.com> wrote:
> >
> > Netfilter code (net/netfilter/nft_log.c and net/netfilter/xt_AUDIT.c)
> > have to be kept in sync. Both source files had duplicated versions of
> > audit_ip4() and audit_ip6() functions, which can result in lack of
> > consistency and/or duplicated work.
> >
> > This patch adds two helper functions in audit.c that can be called by
> > netfilter code commonly, aiming to improve maintainability and
> > consistency.
> >
> > Signed-off-by: Ricardo Robaina <rrobaina@...hat.com>
> > Acked-by: Florian Westphal <fw@...len.de>
> > ---
> > include/linux/audit.h | 12 +++++++++++
> > kernel/audit.c | 39 ++++++++++++++++++++++++++++++++++++
> > net/netfilter/nft_log.c | 43 ++++------------------------------------
> > net/netfilter/xt_AUDIT.c | 43 ++++------------------------------------
> > 4 files changed, 59 insertions(+), 78 deletions(-)
>
> ...
>
> > diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
> > index e35588137995..f53fb4222134 100644
> > --- a/net/netfilter/nft_log.c
> > +++ b/net/netfilter/nft_log.c
> > @@ -26,41 +26,6 @@ struct nft_log {
> > char *prefix;
> > };
> >
> > -static bool audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > -{
> > - struct iphdr _iph;
> > - const struct iphdr *ih;
> > -
> > - ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_iph), &_iph);
> > - if (!ih)
> > - return false;
> > -
> > - audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
> > - &ih->saddr, &ih->daddr, ih->protocol);
> > -
> > - return true;
> > -}
> > -
> > -static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> > -{
> > - struct ipv6hdr _ip6h;
> > - const struct ipv6hdr *ih;
> > - u8 nexthdr;
> > - __be16 frag_off;
> > -
> > - ih = skb_header_pointer(skb, skb_network_offset(skb), sizeof(_ip6h), &_ip6h);
> > - if (!ih)
> > - return false;
> > -
> > - nexthdr = ih->nexthdr;
> > - ipv6_skip_exthdr(skb, skb_network_offset(skb) + sizeof(_ip6h), &nexthdr, &frag_off);
> > -
> > - audit_log_format(ab, " saddr=%pI6c daddr=%pI6c proto=%hhu",
> > - &ih->saddr, &ih->daddr, nexthdr);
> > -
> > - return true;
> > -}
> > -
> > static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
> > {
> > struct sk_buff *skb = pkt->skb;
> > @@ -80,18 +45,18 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
> > case NFPROTO_BRIDGE:
> > switch (eth_hdr(skb)->h_proto) {
> > case htons(ETH_P_IP):
> > - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> > + fam = audit_log_packet_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> > break;
> > case htons(ETH_P_IPV6):
> > - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> > + fam = audit_log_packet_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> > break;
> > }
> > break;
> > case NFPROTO_IPV4:
> > - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> > + fam = audit_log_packet_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> > break;
> > case NFPROTO_IPV6:
> > - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> > + fam = audit_log_packet_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> > break;
> > }
>
> We can probably take this a step further by moving the case statements
> into the audit functions too. I think this will make some of the other
> changes a bit cleaner and should reduce the amount of audit code in the
> NFT code.
>
> If we don't want to do that, it might be worthwhile to take the
> NFPROTO_BRIDGE protocol family reset shown below in audit_log_nft_skb()
> and use that in the nft_log_eval_audit() function so we aren't
> duplicating calls into the audit code.
>
> [WARNING: completely untested code, but you should get the basic idea]
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 26a332ffb1b8..72ba3f51f859 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2538,6 +2538,59 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
> audit_log_end(ab);
> }
>
> +int audit_log_nft_skb(struct audit_buffer *ab,
> + struct sk_buff *skb, u8 nfproto)
> +{
> + /* find the IP protocol in the case of NFPROTO_BRIDGE */
> + if (nfproto == NFPROTO_BRIDGE) {
> + switch (eth_hdr(skb)->h_proto) {
> + case htons(ETH_P_IP):
> + nfproto = NFPROTO_IPV4;
> + case htons(ETH_P_IPV6):
> + nfproto = NFPROTO_IPV6;
> + default:
> + goto unknown_proto;
> + }
> + }
> +
> + switch (nfproto) {
> + case NFPROTO_IPV4: {
> + struct iphdr iph;
> + const struct iphdr *ih;
> +
> + ih = skb_header_pointer(skb, skb_network_offset(skb),
> + sizeof(_iph), &_iph);
> + if (!ih)
> + return -ENOMEM;
> +
> + audit_log_format(ab, " saddr=%pI4 daddr=%pI4 proto=%hhu",
> + &ih->saddr, &ih->daddr, ih->protocol);
> + break;
> + }
> + case NFPROTO_IPV6: {
> + struct ipv6hdr iph;
> + const struct ipv6hdr *ih;
> +
> + ih = skb_header_pointer(skb, skb_network_offset(skb),
> + sizeof(_iph), &_iph);
> + if (!ih)
> + return -ENOMEM;
> +
> + audit_log_format(ab, " saddr=%pI6 daddr=%pI6 proto=%hhu",
> + &ih->saddr, &ih->daddr, ih->protocol);
> + break;
> + }
> + default:
> + goto unknown_proto;
> + }
> +
> + return 0;
> +
> +unknown_proto:
> + audit_log_format(ab, " saddr=? daddr=? proto=?");
> + return -EPFNOSUPPORT;
> +}
> +
> /**
> * audit_set_loginuid - set current task's loginuid
> * @loginuid: loginuid value
> diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
> index e35588137995..6f444e2ad70a 100644
> --- a/net/netfilter/nft_log.c
> +++ b/net/netfilter/nft_log.c
> @@ -75,28 +75,7 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
> return;
>
> audit_log_format(ab, "mark=%#x", skb->mark);
> -
> - switch (nft_pf(pkt)) {
> - case NFPROTO_BRIDGE:
> - switch (eth_hdr(skb)->h_proto) {
> - case htons(ETH_P_IP):
> - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> - break;
> - case htons(ETH_P_IPV6):
> - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> - break;
> - }
> - break;
> - case NFPROTO_IPV4:
> - fam = audit_ip4(ab, skb) ? NFPROTO_IPV4 : -1;
> - break;
> - case NFPROTO_IPV6:
> - fam = audit_ip6(ab, skb) ? NFPROTO_IPV6 : -1;
> - break;
> - }
> -
> - if (fam == -1)
> - audit_log_format(ab, " saddr=? daddr=? proto=-1");
> + audit_log_nft_skb(ab, skb, nft_pf(pkt));
>
> audit_log_end(ab);
> }
>
> --
> paul-moore.com
>
Thanks for reviewing this patch, Paul.
It makes sense to me. I'll work on a newer version addressing your suggestions.
Powered by blists - more mailing lists