netdev - Re: [PATCH v10 nf-next 2/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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8a30aec8-fcc0-4d5f-b4e8-4cb60fe36725@gmail.com>
Date: Wed, 19 Mar 2025 20:18:36 +0100
From: Eric Woudstra <ericwouds@...il.com>
To: Pablo Neira Ayuso <pablo@...filter.org>
Cc: Jozsef Kadlecsik <kadlec@...filter.org>, Roopa Prabhu <roopa@...dia.com>,
 Nikolay Aleksandrov <razor@...ckwall.org>,
 "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>, Shuah Khan <shuah@...nel.org>,
 netfilter-devel@...r.kernel.org, bridge@...ts.linux.dev,
 netdev@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v10 nf-next 2/3] netfilter: nft_chain_filter: Add bridge
 double vlan and pppoe

On 3/19/25 12:04 AM, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Sat, Mar 15, 2025 at 09:00:32PM +0100, Eric Woudstra wrote:
>> This adds the capability to evaluate 802.1ad, QinQ, PPPoE and PPPoE-in-Q
>> packets in the bridge filter chain.
>>
>> Reviewed-by: Nikolay Aleksandrov <razor@...ckwall.org>
>> Signed-off-by: Eric Woudstra <ericwouds@...il.com>
>> ---
>>  net/netfilter/nft_chain_filter.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
>> index 19a553550c76..7c7080c1a67d 100644
>> --- a/net/netfilter/nft_chain_filter.c
>> +++ b/net/netfilter/nft_chain_filter.c
>> @@ -232,11 +232,27 @@ nft_do_chain_bridge(void *priv,
>>  		    struct sk_buff *skb,
>>  		    const struct nf_hook_state *state)
>>  {
>> +	struct ethhdr *ethh = eth_hdr(skb);
>>  	struct nft_pktinfo pkt;
>> +	int thoff;
>>  
>>  	nft_set_pktinfo(&pkt, skb, state);
>>  
>> -	switch (eth_hdr(skb)->h_proto) {
>> +	switch (ethh->h_proto) {
>> +	case htons(ETH_P_PPP_SES):
>> +		thoff = PPPOE_SES_HLEN;
>> +		ethh += thoff;
> 
> This pointer arithmetics does not look correct, ethh is struct ethhdr,
> neither void nor char.
> 
>> +		break;
>> +	case htons(ETH_P_8021Q):
>> +		thoff = VLAN_HLEN;
>> +		ethh += thoff;
> 
> Same here.
> 
>> +		break;
>> +	default:
>> +		thoff = 0;
>> +		break;
>> +	}
>> +
>> +	switch (ethh->h_proto) {
> 
> This switch will match on the wrong offset.
> 
>>  	case htons(ETH_P_IP):
>>  		nft_set_pktinfo_ipv4_validate(&pkt);
>>  		break;
>> @@ -248,6 +264,8 @@ nft_do_chain_bridge(void *priv,
>>  		break;
>>  	}
>>  
>> +	pkt.thoff += thoff;
> 
> And only transport offset is adjusted here.
> 
>>  	return nft_do_chain(&pkt, priv);
>>  }
>>  
>> -- 
>> 2.47.1
>>

I will sort this out and send a new version after the merge window.