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

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

[Yun Zhou]

__skb_flow_dissect() unconditionally reads 12 bytes from eth_hdr(skb)
when FLOW_DISSECTOR_KEY_ETH_ADDRS is requested. This assumes the skb
has a valid Ethernet header at mac_header, which is not always the case.

The problem can be triggered by:
 1. Creating a TUN device in L3 mode (IFF_TUN, hard_header_len=0)
 2. Attaching a multiq qdisc with a flower filter matching on eth_src
 3. Sending a packet through AF_PACKET

Since TUN in L3 mode has no link-layer header, mac_header points to
the L3 data area. The flow dissector reads 12 bytes of uninitialized
skb memory, which then propagates through fl_set_masked_key() and is
used as a rhashtable lookup key in __fl_lookup(), as reported by KMSAN.

Rejecting the filter in the control path (at tc filter add time) is
not feasible because TC filter blocks can be shared between arbitrary
devices -- a filter installed on an Ethernet device may later classify
packets on a headerless device through a shared block. The device
association is not fixed at filter creation time.

Fix this in the data path by checking skb->dev->hard_header_len before
reading. 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.

Reported-by: syzbot+fa2f5b1fb06147be5e16@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fa2f5b1fb06147be5e16
Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses")
Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
v2:
 - Adjust commit message and comment

 net/core/flow_dissector.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 2a98f5fa74eb..1e6a2c04698c 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))
+			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,
-- 
2.43.0

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

[Jakub Kicinski]

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