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

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

[Felix Fietkau]

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;
 
 	/* convert IEEE 802.11 header + possible LLC headers into Ethernet
 	 * header
@@ -574,8 +572,9 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr,
 	memcpy(tmp.h_dest, ieee80211_get_DA(hdr), ETH_ALEN);
 	memcpy(tmp.h_source, ieee80211_get_SA(hdr), ETH_ALEN);
 
-	if (iftype == NL80211_IFTYPE_MESH_POINT)
-		skb_copy_bits(skb, hdrlen, &mesh_flags, 1);
+	if (iftype == NL80211_IFTYPE_MESH_POINT &&
+	    skb_copy_bits(skb, hdrlen, &mesh_flags, 1) < 0)
+		return -1;
 
 	mesh_flags &= MESH_FLAGS_AE;
 
@@ -595,11 +594,12 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr,
 		if (iftype == NL80211_IFTYPE_MESH_POINT) {
 			if (mesh_flags == MESH_FLAGS_AE_A4)
 				return -1;
-			if (mesh_flags == MESH_FLAGS_AE_A5_A6) {
-				skb_copy_bits(skb, hdrlen +
-					offsetof(struct ieee80211s_hdr, eaddr1),
-					tmp.h_dest, 2 * ETH_ALEN);
-			}
+			if (mesh_flags == MESH_FLAGS_AE_A5_A6 &&
+			    skb_copy_bits(skb, hdrlen +
+					  offsetof(struct ieee80211s_hdr, eaddr1),
+					  tmp.h_dest, 2 * ETH_ALEN) < 0)
+				return -1;
+
 			hdrlen += __ieee80211_get_mesh_hdrlen(mesh_flags);
 		}
 		break;
@@ -613,10 +613,11 @@ int ieee80211_data_to_8023_exthdr(struct sk_buff *skb, struct ethhdr *ehdr,
 		if (iftype == NL80211_IFTYPE_MESH_POINT) {
 			if (mesh_flags == MESH_FLAGS_AE_A5_A6)
 				return -1;
-			if (mesh_flags == MESH_FLAGS_AE_A4)
-				skb_copy_bits(skb, hdrlen +
-					offsetof(struct ieee80211s_hdr, eaddr1),
-					tmp.h_source, ETH_ALEN);
+			if (mesh_flags == MESH_FLAGS_AE_A4 &&
+			    skb_copy_bits(skb, hdrlen +
+					  offsetof(struct ieee80211s_hdr, eaddr1),
+					  tmp.h_source, ETH_ALEN) < 0)
+				return -1;
 			hdrlen += __ieee80211_get_mesh_hdrlen(mesh_flags);
 		}
 		break;
@@ -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;
 		skb_postpull_rcsum(skb, &payload, ETH_ALEN + 2);
 	} else {
 		tmp.h_proto = htons(skb->len - hdrlen);
-- 
2.36.1

[PATCH 6.1 2/2] wifi: mac80211: do not drop packets smaller than the LLC-SNAP header on fast-rx

[Felix Fietkau]

Since STP TCN frames are only 7 bytes, the pskb_may_pull call returns an error.
Instead of dropping those packets, bump them back to the slow path for proper
processing.

Fixes: 49ddf8e6e234 ("mac80211: add fast-rx path")
Reported-by: Chad Monroe <chad.monroe@smartrg.com>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/mac80211/rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 9183e5f1ad29..589521717c35 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4709,7 +4709,7 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
 
 	if (!(status->rx_flags & IEEE80211_RX_AMSDU)) {
 		if (!pskb_may_pull(skb, snap_offs + sizeof(*payload)))
-			goto drop;
+			return false;
 
 		payload = (void *)(skb->data + snap_offs);
 
-- 
2.36.1

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

[Johannes Berg]

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