netdev - Re: [PATCH v14 nf-next 2/3] netfilter: bridge: Add conntrack 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aG2VDyHfVsp5L2zR@strlen.de>
Date: Wed, 9 Jul 2025 00:00:47 +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 2/3] netfilter: bridge: Add conntrack double
 vlan and pppoe

Eric Woudstra <ericwouds@...il.com> wrote:
> This adds the capability to conntrack 802.1ad, QinQ, PPPoE and PPPoE-in-Q
> packets that are passing a bridge, only when a conntrack zone is set.
> 
> Signed-off-by: Eric Woudstra <ericwouds@...il.com>
> ---
>  net/bridge/netfilter/nf_conntrack_bridge.c | 88 ++++++++++++++++++----
>  1 file changed, 72 insertions(+), 16 deletions(-)
> 
> +			data_len = ntohs(ph->hdr.length) - 2;

Shouldn't there be some validation on data_len here?

> +		if (!pskb_may_pull(skb, offset + sizeof(struct iphdr)))
> +			goto do_not_track;
 
>  		len = skb_ip_totlen(skb);
> -		if (pskb_trim_rcsum(skb, len))
> -			return NF_ACCEPT;
> +		if (data_len < len)
> +			len = data_len;

Hmm.  So if ph->hdr.length is smaller than what ip header claims,
len shrinks.

If its higher, then the mismatch is ignored and we only use
the ip header length (i.e., the smaller value).

> +		if (pskb_trim_rcsum(skb, offset + len))
> +			goto do_not_track;

Is the intent that garbage data_len is caught here and

>  		if (nf_ct_br_ip_check(skb))
> -			return NF_ACCEPT;

here?  If so, maybe a comment could help.

> +		goto do_not_track;
>  	}
>  
> -	if (ret != NF_ACCEPT)
> -		return ret;
> +	if (ret == NF_ACCEPT)
> +		ret = nf_conntrack_in(skb, &bridge_state);
>  
> -	return nf_conntrack_in(skb, &bridge_state);
> +do_not_track:
> +	if (offset) {

if (ret == NF_ACCEPT && offset) { ...

Else skb could have been free'd. There should be test cases for this
functionality included.  If we lack test cases for the existing
functionality, which might be the case, please consider submitting
a reduced test case first so it can be applied regardless of the
remaining functionality.