netdev - Re: [PATCH v14 nf-next 3/3] netfilter: nft_chain_filter: Add bridge double vlan and pppoe

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aLbuokOe9zcN27sd@strlen.de>
Date: Tue, 2 Sep 2025 15:18:26 +0200
From: Florian Westphal <fw@...len.de>
To: Eric Woudstra <ericwouds@...il.com>
Cc: Pablo Neira Ayuso <pablo@...filter.org>,
	Jozsef Kadlecsik <kadlec@...filter.org>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Ido Schimmel <idosch@...dia.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, netfilter-devel@...r.kernel.org,
	bridge@...ts.linux.dev, netdev@...r.kernel.org
Subject: Re: [PATCH v14 nf-next 3/3] netfilter: nft_chain_filter: Add bridge
 double vlan and pppoe

Eric Woudstra <ericwouds@...il.com> wrote:
> > Thats because of implicit dependency insertion on userspace side:
> > # ip saddr 1.2.3.4 counter ip daddr 3.4.5.6
> > bridge test-bridge input
> >   [ meta load protocol => reg 1 ]
> >   [ cmp eq reg 1 0x00000008 ]
> >   [ payload load 4b @ network header + 12 => reg 1 ]
> >   ...
> > 
> > So, if userspace would NOT do that it would 'just work'.
> > 
> > Pablo, whats your take on this?
> > We currently don't have a 'nhproto' field in nft_pktinfo
> > and there is no space to add one.
> > 
> > We could say that things work as expected, and that
> >  ip saddr 1.2.3.4
> > 
> > should not magically match packets in e.g. pppoe encap.

FTR, I think 'ip saddr 1.2.3.4' (standalone with no other info),
should NOT match inside a random l2 tunnel.

> > I suspect it will start to work if you force it to match in pppoe, e.g.
> > ether type 0x8864 ip saddr ...
> > 
> > so nft won't silently add the skb->protocol dependency.
> > 
> > Its not a technical issue but about how matching is supposed to work
> > in a bridge.
> > 
> > If its supposed to work automatically we need to either:
> > 1. munge skb->protocol in kernel, even tough its wrong (we don't strip
> >    the l2 headers).
> > 2. record the real l3 protocol somewhere and make it accessible, then
> >    fix the dependency generation in userspace to use the 'new way' (meta
> >    l3proto)?
> > 3. change the dependency generation to something else.
> >    But what? 'ether type ip' won't work either for 8021ad etc.
> >    'ip version' can't be used for arp.
> > 
> 
> Hi Florian,
> 
> Did you get any information on how to handle this issue?

Did you check if you can get it to match if you add the relevant
l3 dependency in the rule?

I don't think we should (or can) change how the rules get evaluated by
making 'ip saddr' match on other l2 tunnel protocols by default.

It is even incompatible with any exiting rulesets, consider e.g.
"ip daddr 1.2.3.4 drop" on a bridge, now this address becomes
unreachable but it works before your patch (if the address is found in
e.g. pppoe header).

'ip/ip6' should work as expected as long as userspace provides
the correct ether type and dependencies.

I.e., what this patch adds as C code should work if being provided
as part of the rule.

What might make sense is to add the ppp(oe) header to src/proto.c
in nftables so users that want to match the header following ppp
one don't have to use raw payload match syntax.

What might also make sense is to either add a way to force a call
to nft_set_pktinfo_ipv4_validate() from the ruleset, or take your
patch but WITHOUT the skb->protocol munging.

However, due to the number of possible l2 header chain combinations
I'm not sure we should bother with trying to add all of them.

I worry we would end up turning nft_do_chain_bridge() preamble or
nft_set_pktinfo() into some kind of l2 packet dissector.

Maybe one way forward is to introduce

	NFT_META_BRI_INET_VALIDATE

nft add rule ... meta inet validate ...
(just an idea, come up with better names...)

We'd have to add NFT_PKTINFO_L3PROTO flag to
include/net/netfilter/nf_tables.h.
(or, alternatively NFT_PKTINFO_UNSPEC).

Then, set this flag in struct nft_pktinfo, from
nft_set_pktinfo_ipv4|6_validate (or from nft_set_pktinfo_unspec).

NFT_META_BRI_INET_VALIDATE, would call nft_set_pktinfo_ipv4_validate
or nft_set_pktinfo_ipv6_validate depending on iph->version and set
NFT_BREAK verdict if the flag is still absent.

**USERSPACE IS RESPONSIBLE** to prevent arp packets from entering
this expression. If they do, then header validation should fail
but there would be an off-chance that the garbage is also a valid
ipv4 or ipv6 packet.