Re: [PATCH v3] 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 patch addresses a KMSAN report by gating the read of eth_hdr(skb)
in __skb_flow_dissect() on skb->dev->hard_header_len being at least the
size of the ETH_ADDRS dissector key, zeroing the key otherwise. The
goal is to avoid reading uninitialized memory when a TUN device in
IFF_TUN (L3) mode passes a packet without a link-layer header to a
flower filter on a shared block.

Closes: https://syzkaller.appspot.com/bug?extid=fa2f5b1fb06147be5e16

> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 2a98f5fa74eb..de6ca9beee03 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->dev &&
> +		    skb->dev->hard_header_len >= sizeof(*key_eth_addrs))
> +			memcpy(key_eth_addrs, eth_hdr(skb), sizeof(*key_eth_addrs));

[Medium]
Is hard_header_len the right predicate to gate this read on?

The dissector key is ETH_ALEN+ETH_ALEN, so the gate accepts any device
with hard_header_len >= 12. IPoIB sets hard_header_len = IPOIB_HARD_LEN
with dev->type = ARPHRD_INFINIBAND, and ARPHRD_FDDI uses 21 bytes.
Wouldn't packets on those link types pass this check and have the first
12 bytes of a non-Ethernet L2 header copied into key_eth_addrs as if
they were the destination and source MAC addresses?

Would a check on dev->type == ARPHRD_ETHER (or dev->addr_len == ETH_ALEN)
be more appropriate, so that only true Ethernet framing is read into
the key?

[Medium]
Does this gate behave correctly across act_mirred?

act_mirred reassigns skb->dev to the mirror/redirect target before the
target device's classifier path runs. Consider a packet originally
framed on an Ethernet device, with a valid mac_header in the linear
region, redirected to a headerless device such as TUN IFF_TUN that
shares a TC block with the source. The key_eth_addrs would now be
zeroed via the else branch even though the original Ethernet header
is still present in the skb. Is that intended for shared blocks?

[Medium]
Does this still leave a path where eth_hdr(skb) reads past the buffer?

eth_hdr(skb) expands to skb->head + skb->mac_header. When mac_header is
the unset sentinel (typeof(skb->mac_header))~0U set by skb allocation
or by skb_unset_mac_header(), the dereference resolves to skb->head +
0xFFFF, well past skb_end_pointer(skb). The new gate only inspects
skb->dev->hard_header_len, which is independent of skb->mac_header, so
any skb delivered to the dissector with mac_header unset on a device
whose hard_header_len >= 12 would still read out-of-bounds bytes into
key_eth_addrs.

Would skb_mac_header_was_set(skb) plus a bounds check that the 12 bytes
lie within [skb->head, skb_tail_pointer(skb)) be a more direct match
for the property the commit message describes:

    This assumes the skb has a valid Ethernet header at mac_header,
    which is not always the case.

> +		else
> +			memset(key_eth_addrs, 0, sizeof(*key_eth_addrs));

[Low]
The commit message says:

    If the device does not have a link-layer header large enough to
    contain the Ethernet addresses, zero the key so the filter will
    not match.

Is that wording accurate for all filter shapes?

__fl_lookup() uses key_eth_addrs as part of the rhashtable lookup key.
A flower filter installed to match src_mac 00:00:00:00:00:00 and
dst_mac 00:00:00:00:00:00 (for example a catch-all on a shared block)
would, after this change, deterministically match every packet on a
headerless device, where previously the random skb memory made such a
filter effectively never match there.

Could the commit message note this corner case, or could a sentinel
value that cannot collide with a legal user-supplied key be used
instead of all zeros?

>  	}
>  
>  	if (dissector_uses_key(flow_dissector,
-- 
pw-bot: cr