netdev - Re: [PATCH v12 nf-next 1/2] 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: <9866f2d2-eda8-470f-99fb-5a8d6756de56@gmail.com>
Date: Sat, 28 Jun 2025 15:27:46 +0200
From: Eric Woudstra <ericwouds@...il.com>
To: Florian Westphal <fw@...len.de>
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 v12 nf-next 1/2] netfilter: bridge: Add conntrack double
 vlan and pppoe

On 6/22/25 10:16 PM, Florian Westphal wrote:
> Eric Woudstra <ericwouds@...il.com> wrote:
>> -	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) {
>> +		__skb_push(skb, offset);
> 
> nf_conntrack_in() can free the skb, or steal it.
> 
> But aside from this, I'm not sure this is a good idea to begin with,
> it feels like we start to reimplement br_netfilter.c .
> 
> Perhaps it would be better to not push/pull but instead rename
> 
> unsigned int
> nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
> 
>  to
> 
> unsigned int 
> nf_conntrack_inner(struct sk_buff *skb, const struct nf_hook_state *state,
> 		   unsigned int nhoff)
> 
> and add
> 
> unsigned int 
> nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
> {
> 	return nf_conntrack_inner(skb, state, skb_network_offset(skb));
> }
> 
> Or, alternatively, add
> struct nf_ct_pktoffs {
> 	u16 nhoff;
> 	u16 thoff;
> };
> 
> then populate that from nf_ct_bridge_pre(), then pass that to
> nf_conntrack_inner() (all names are suggestions, if you find something
> better thats fine).
> 
> Its going to be more complicated than this, but my point is that e.g.
> nf_ct_get_tuple() already gets the l4 offset, so why not pass l3
> offset too?

So I've tried nf_conntrack_inner(). The thing is:

>  	switch (skb->protocol) {
>  	case htons(ETH_P_IP):
>  		if (!pskb_may_pull(skb, sizeof(struct iphdr)))
> -			return NF_ACCEPT;
> +			goto do_not_track;
>
>  		len = skb_ip_totlen(skb);
> +		if (data_len < len)
> +			len = data_len;
>  		if (pskb_trim_rcsum(skb, len))
> -			return NF_ACCEPT;
> +			goto do_not_track;
>
>  		if (nf_ct_br_ip_check(skb))
> -			return NF_ACCEPT;
> +			goto do_not_track;
>
>  		bridge_state.pf = NFPROTO_IPV4;
>  		ret = nf_ct_br_defrag4(skb, &bridge_state);
>  		break;
>  	case htons(ETH_P_IPV6):
>  		if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
> -			return NF_ACCEPT;
> +			goto do_not_track;
>
>  		len = sizeof(struct ipv6hdr) + ntohs(ipv6_hdr(skb)->payload_len);
> +		if (data_len < len)
> +			len = data_len;
>  		if (pskb_trim_rcsum(skb, len))
> -			return NF_ACCEPT;
> +			goto do_not_track;
>
>  		if (nf_ct_br_ipv6_check(skb))
> -			return NF_ACCEPT;
> +			goto do_not_track;
>
>  		bridge_state.pf = NFPROTO_IPV6;
>  		ret = nf_ct_br_defrag6(skb, &bridge_state);
>  		break;

This part all use ip_hdr(skb) and ipv6_hdr(skb). I could add offset to
skb->network_header temporarily for this part of the code. Do you think
that is okay?

Adding offset to skb->network_header during the call to
nf_conntrack_in() does not work, but, as you mentioned, adding the
offset through the nf_conntrack_inner() function, that does work. Except
for 1 piece of code, I found so far:

nf_checksum() reports an error when it is called from
nf_conntrack_tcp_packet(). It also uses ip_hdr(skb) and ipv6_hdr(skb).
Strangely, It only gives the error when dealing with a pppoe packet or
pppoe-in-q packet. There is no error when q-in-q (double q) or 802.1ad
are involved.

Do you have any suggestion how you want to handle this failure in
nf_checksum()?