Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <972da18c889300252656aa53cc8d9e70d1e90174.camel@sipsolutions.net>
Subject: Re: [PATCH 6.1 1/2] wifi: cfg80211: fix
 ieee80211_data_to_8023_exthdr handling of small packets
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Felix Fietkau <nbd@nbd.name>, linux-wireless@vger.kernel.org
Cc:     Chad Monroe <chad.monroe@smartrg.com>
Date:   Fri, 07 Oct 2022 14:38:16 +0200
In-Reply-To: <20221007090509.18503-1-nbd@nbd.name>
References: <20221007090509.18503-1-nbd@nbd.name>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

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 c=
ode
> on the skb_copy_bits calls.
>=20
> Fixes: 2d1c304cb2d5 ("cfg80211: add function for 802.3 conversion with se=
parate 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(-)
>=20
> 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;
> =20
>  	hdrlen =3D 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 *s=
kb, struct ethhdr *ehdr,
>  		break;
>  	}
> =20
> -	skb_copy_bits(skb, hdrlen, &payload, sizeof(payload));
> -	tmp.h_proto =3D payload.proto;
> -
> -	if (likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) =
&&
> +	if (likely(skb_copy_bits(skb, hdrlen, &payload, sizeof(payload)) =3D=3D=
 0) &&
> +	    likely((!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_header) =
&&
>  		    tmp.h_proto !=3D htons(ETH_P_AARP) &&
>  		    tmp.h_proto !=3D htons(ETH_P_IPX)) ||
>  		   ether_addr_equal(payload.hdr, bridge_tunnel_header))) {
>  		/* remove RFC1042 or Bridge-Tunnel encapsulation and
>  		 * replace EtherType */
>  		hdrlen +=3D ETH_ALEN + 2;
> +		tmp.h_proto =3D 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)) =
=3D=3D 0 &&
+                  (!is_amsdu && ether_addr_equal(payload.hdr, rfc1042_head=
er) &&
+                   payload.proto !=3D htons(ETH_P_AARP) &&
+                   payload.proto !=3D htons(ETH_P_IPX)) ||


but better if you check and resubmit.

johannes