Re: [PATCH 6.1 1/2] wifi: cfg80211: fix ieee80211_data_to_8023_exthdr handling of small packets

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2022-10-07 at 11:05 +0200, Felix Fietkau wrote:
> STP topology change notification packets only have a payload of 7 bytes,
> so they get dropped due to the skb->len < hdrlen + 8 check.
> Fix this by removing skb->len based checks and instead check the return code
> on the skb_copy_bits calls.
> 
> Fixes: 2d1c304cb2d5 ("cfg80211: add function for 802.3 conversion with separate output buffer")
> Reported-by: Chad Monroe <chad.monroe@smartrg.com>
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  net/wireless/util.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 01493568a21d..35f630c6de11 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -559,8 +559,6 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr,
>  		return -1;
>  
>  	hdrlen = ieee80211_hdrlen(hdr->frame_control) + data_offset;
> -	if (skb->len < hdrlen + 8)
> -		return -1;

Can't decide if I'm happy with that vs. just dropping the "+ 8" ...

On the one hand, we already assume at least that the header is in the
linear portion for when we access it, but on the other hand it feels
safer to keep the check?

> @@ -628,16 +629,15 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr,
>  		break;
>  	}
>  
> -	skb_copy_bits(skb, hdrlen, &payload, sizeof(payload));
> -	tmp.h_proto = payload.proto;
> -
> -	if (likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) &&
> +	if (likely(skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)) == 0) &&
> +	    likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) &&
>  		    tmp.h_proto != htons(ETH_P_AARP) &&
>  		    tmp.h_proto != htons(ETH_P_IPX)) ||
>  		   ether_addr_equal(payload.hdr, bridge_tunnel_header))) {
>  		/* remove RFC1042 or Bridge-Tunnel encapsulation and
>  		 * replace EtherType */
>  		hdrlen += ETH_ALEN + 2;
> +		tmp.h_proto = payload.proto;

As you can see on the last line, this is broken.

Double likely() also seems weird.

I think it needs to be

+       if (likely(skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)) == 0 &&
+                  (!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) &&
+                   payload.proto != htons(ETH_P_AARP) &&
+                   payload.proto != htons(ETH_P_IPX)) ||


but better if you check and resubmit.

johannes