[PATCH] mac80211: don't send invalid QoS frames

[PATCH] mac80211: don't send invalid QoS frames

[Johannes Berg]

Kalle Valo noticed that QoS frames are sent with an invalid QoS control
field; this is because we increase the header length but neither
initialise the space nor actually have enough space in the header
structure for the QoS control field.

This patch fixes it by treating the QoS field specially and appending it
explicitly, initialising it to zero.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
Kalle, please check if this fixes the weird frames you saw. I think it
will but would like to be sure. If it does, please say so and then we
should merge this patch to 2.6.24 and wireless-dev and I may make one
for -stable as well.

I think this is nicer than my previous patch because it explicitly
appends the QoS field and gives us a place to modify it should we ever
want.

 net/mac80211/tx.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

--- wireless-dev.orig/net/mac80211/tx.c	2007-08-30 14:35:21.152051253 +0200
+++ wireless-dev/net/mac80211/tx.c	2007-08-30 16:16:17.595374680 +0200
@@ -1493,7 +1493,20 @@ int ieee80211_subif_start_xmit(struct sk
 		nh_pos += encaps_len;
 		h_pos += encaps_len;
 	}
-	memcpy(skb_push(skb, hdrlen), &hdr, hdrlen);
+
+	if (fc & IEEE80211_STYPE_QOS_DATA) {
+		__le16 *qos_control;
+
+		memcpy(skb_push(skb, hdrlen - 2), &hdr, hdrlen - 2);
+		qos_control = (__le16*) skb_push(skb, 2);
+		/*
+		 * Maybe we could actually set some fields here, for now just
+		 * initialise to zero to indicate no special operation.
+		 */
+		*qos_control = 0;
+	} else
+		memcpy(skb_push(skb, hdrlen), &hdr, hdrlen);
+
 	nh_pos += hdrlen;
 	h_pos += hdrlen;
 

Re: [PATCH] mac80211: don't send invalid QoS frames

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> Kalle Valo noticed that QoS frames are sent with an invalid QoS control
> field; this is because we increase the header length but neither
> initialise the space nor actually have enough space in the header
> structure for the QoS control field.
>
> This patch fixes it by treating the QoS field specially and appending it
> explicitly, initialising it to zero.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>
> ---
> Kalle, please check if this fixes the weird frames you saw. I think it
> will but would like to be sure. If it does, please say so and then we
> should merge this patch to 2.6.24 and wireless-dev and I may make one
> for -stable as well.

Now b43 won't send any unicast frames to N800 according to a wireless
sniffer (and packet loss is 100%, obviously). Broadcast frames are
sent and N800 replies to ARP requests just fine.

I was a bit surprised by the result, so I double checked my tests.
After reverting this patch unicast frames were sent again, just that
QoS frames were broken as expected. So this patch definetely broke
unicast QoS send.

Tested with wireless-dev#anything commit 60faa0c2 and N800 had PSM
disabled. I didn't have time to test with QoS disabled, sorry. But I
can test that later, if you want.

-- 
Kalle Valo

Re: [PATCH] mac80211: don't send invalid QoS frames

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On Fri, 2007-08-31 at 11:43 +0300, Kalle Valo wrote:

> Now b43 won't send any unicast frames to N800 according to a wireless
> sniffer (and packet loss is 100%, obviously). Broadcast frames are
> sent and N800 replies to ARP requests just fine.
> 
> I was a bit surprised by the result, so I double checked my tests.
> After reverting this patch unicast frames were sent again, just that
> QoS frames were broken as expected. So this patch definetely broke
> unicast QoS send.

Huh, ok, I'll take another look.

> Tested with wireless-dev#anything commit 60faa0c2 and N800 had PSM
> disabled. I didn't have time to test with QoS disabled, sorry. But I
> can test that later, if you want.

I don't think it matters much. You should rather look into why the N800
is accepting those bogus frames :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH] mac80211: don't send invalid QoS frames

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

Oh, I see a bug.

On Thu, 2007-08-30 at 16:22 +0200, Johannes Berg wrote:

> +	if (fc & IEEE80211_STYPE_QOS_DATA) {
> +		__le16 *qos_control;
> +
> +		memcpy(skb_push(skb, hdrlen - 2), &hdr, hdrlen - 2);
> +		qos_control = (__le16*) skb_push(skb, 2);

These last two statements should be the other way around, of course.
We're pushing data into the head of the skb here, so we need to push the
QoS control field first and then the header. The way I wrote it there
the zeroed field ends up being the frame control field, indicating that
it's an association request frame. You should be seeing those as mangled
frames in wireshark though.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH v2] mac80211: don't send invalid QoS frames

[Johannes Berg]

Subject: [PATCH] mac80211: don't send invalid QoS frames

Kalle Valo noticed that QoS frames are sent with an invalid QoS control
field; this is because we increase the header length but neither
initialise the space nor actually have enough space in the header
structure for the QoS control field.

This patch fixes it by treating the QoS field specially and appending it
explicitly, initialising it to zero.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
Kalle, please check if this fixes the weird frames you saw. I think it
will but would like to be sure. If it does, please say so and then we
should merge this patch to 2.6.24 and wireless-dev and I may make one
for -stable as well.

I think this is nicer than my previous patch because it explicitly
appends the QoS field and gives us a place to modify it should we ever
want.

 net/mac80211/tx.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

--- wireless-dev.orig/net/mac80211/tx.c	2007-08-31 04:20:21.982792130 +0200
+++ wireless-dev/net/mac80211/tx.c	2007-08-31 13:26:22.252784387 +0200
@@ -1493,7 +1493,20 @@ int ieee80211_subif_start_xmit(struct sk
 		nh_pos += encaps_len;
 		h_pos += encaps_len;
 	}
-	memcpy(skb_push(skb, hdrlen), &hdr, hdrlen);
+
+	if (fc & IEEE80211_STYPE_QOS_DATA) {
+		__le16 *qos_control;
+
+		qos_control = (__le16*) skb_push(skb, 2);
+		memcpy(skb_push(skb, hdrlen - 2), &hdr, hdrlen - 2);
+		/*
+		 * Maybe we could actually set some fields here, for now just
+		 * initialise to zero to indicate no special operation.
+		 */
+		*qos_control = 0;
+	} else
+		memcpy(skb_push(skb, hdrlen), &hdr, hdrlen);
+
 	nh_pos += hdrlen;
 	h_pos += hdrlen;