Re: [PATCH v2] flow_dissector: fix uninit-value in __skb_flow_dissect() for ETH_ADDRS

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
flow_dissector: fix uninit-value in __skb_flow_dissect() for ETH_ADDRS
    
This commit attempts to fix an uninitialized value read in __skb_flow_dissect()
for ETH_ADDRS. It checks the packet length before parsing the MAC addresses
to prevent issues on headerless L3 devices.

Reported-by: syzbot+fa2f5b1fb06147be5e16@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fa2f5b1fb06147be5e16
Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")

> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 2a98f5fa74eb0..1e6a2c04698ca 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c

[ ... ]

> @@ -1173,13 +1173,20 @@ bool __skb_flow_dissect(const struct net *net,
>  
>  	if (dissector_uses_key(flow_dissector,
>  			       FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> -		struct ethhdr *eth = eth_hdr(skb);
>  		struct flow_dissector_key_eth_addrs *key_eth_addrs;
>  
>  		key_eth_addrs = skb_flow_dissector_target(flow_dissector,
>  							  FLOW_DISSECTOR_KEY_ETH_ADDRS,
>  							  target_container);
> -		memcpy(key_eth_addrs, eth, sizeof(*key_eth_addrs));
> +		/* TC filter blocks can be shared across devices with
> +		 * different header lengths, so we cannot validate this
> +		 * when the filter is installed -- check at dissect time.
> +		 */
> +		if (skb_mac_header_was_set(skb) &&
> +		    skb_tail_pointer(skb) - skb_mac_header(skb) >= sizeof(*key_eth_addrs))

[Severity: High]
Does this logic bypass the bounds check if the MAC header points past the tail?

The pointer subtraction yields a signed value, which is then compared against
sizeof(*key_eth_addrs), an unsigned size_t. If an SKB is modified such that
skb->mac_header points past skb->tail, the subtraction results in a negative
value. Due to C integer promotion rules, this promotes to a massive unsigned
value, bypassing the length check.

Would it be safer to use pointer addition instead, such as:
skb_mac_header(skb) + sizeof(*key_eth_addrs) <= skb_tail_pointer(skb)?

[Severity: High]
The commit message mentions fixing this by checking skb->dev->hard_header_len,
but it appears this check is missing from the implemented code.

For L3 devices like TUN, mac_header typically points to the start of the L3
payload, like the IPv4 header. Since most IP packets are larger than 12 bytes,
this length condition will evaluate to true without checking the hardware
header length.

Will this cause the flow dissector to incorrectly copy the first 12 bytes of
the IP header into the Ethernet addresses key?

> +			memcpy(key_eth_addrs, eth_hdr(skb), sizeof(*key_eth_addrs));
> +		else
> +			memset(key_eth_addrs, 0, sizeof(*key_eth_addrs));
>  	}
>  
>  	if (dissector_uses_key(flow_dissector,
-- 
pw-bot: cr