Skip to contentSkip to conversation

Staging: wlan-ng: memsetting the wrong amount of data

PATCH In review Dan Carpenter 3 participants 5 messages Feb 21, 2012 – Feb 23, 2012

Subsystems (2)
Staging Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 Staging: wlan-ng: memsetting the wrong amount of data

p80211item_pstr6_t is the size of "msg1.bssid" (16 bytes) but msg1.bssid.data is type p80211pstr6_t and it is smaller (7 bytes). We had just set that memory to zeroes earlier and now we're writing over it with 0xff because we're writing past the end of the struct.

I don't know if this actually causes a problem. It may be that we initialize the extra 0xff bytes correctly later. But the current code is obviously wrong and we should fix it.

Signed-off-by Dan Carpenter <[email protected]>
drivers/staging/wlan-ng/cfg80211.c +1 -1
index fb466f4..d518b31 100644
@@ -356,7 +356,7 @@ int prism2_scan(struct wiphy *wiphy, struct net_device *dev,
 	msg1.msgcode = DIDmsg_dot11req_scan;
 	msg1.bsstype.data = P80211ENUM_bsstype_any;
 
-	memset(&(msg1.bssid.data), 0xFF, sizeof(p80211item_pstr6_t));
+	memset(&msg1.bssid.data, 0xFF, sizeof(msg1.bssid.data));
 	msg1.bssid.data.len = 6;
 
 	if (request->n_ssids > 0) {
#1 lore raw

Am 21.02.2012 15:18, schrieb Dan Carpenter:

Quoted 16 lines

p80211item_pstr6_t is the size of "msg1.bssid" (16 bytes) but msg1.bssid.data is type p80211pstr6_t and it is smaller (7 bytes). We had just set that memory to zeroes earlier and now we're writing over it with 0xff because we're writing past the end of the struct.

I don't know if this actually causes a problem. It may be that we initialize the extra 0xff bytes correctly later. But the current code is obviously wrong and we should fix it.

Signed-off-by Dan Carpenter <[email protected]>
drivers/staging/wlan-ng/cfg80211.c +0 -0
index fb466f4..d518b31 100644

drivers/staging/wlan-ng/cfg80211.c int prism2_scan(struct wiphy *wiphy, struct net_device *dev, · line 360

Quoted 6 lines
drivers/staging/wlan-ng/cfg80211.c +1 -1
@@ -356,7 +356,7 @@ int prism2_scan(struct wiphy *wiphy, struct net_device *dev,
 	msg1.msgcode = DIDmsg_dot11req_scan;
 	msg1.bsstype.data = P80211ENUM_bsstype_any;
 
-	memset(&(msg1.bssid.data), 0xFF, sizeof(p80211item_pstr6_t));
+	memset(&msg1.bssid.data, 0xFF, sizeof(msg1.bssid.data));
 	msg1.bssid.data.len = 6;

maybe msg1.bssid.data.len is related to msg1.bssid.data ? I guess sizeof(msg1.bssid.data)-1 (why -1).

perhaps you can fix both ?

re,
wh

Quoted 7 lines
drivers/staging/wlan-ng/cfg80211.c +0 -1
 	if (request->n_ssids > 0) {
--

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 9 lines · walter harms

drivers/staging/wlan-ng/cfg80211.c int prism2_scan(struct wiphy *wiphy, struct net_device *dev, · line 360

Quoted 3 lines
drivers/staging/wlan-ng/cfg80211.c +1 -1
-	memset(&(msg1.bssid.data), 0xFF, sizeof(p80211item_pstr6_t));
+	memset(&msg1.bssid.data, 0xFF, sizeof(msg1.bssid.data));
 	msg1.bssid.data.len = 6;

maybe msg1.bssid.data.len is related to msg1.bssid.data ? I guess sizeof(msg1.bssid.data)-1 (why -1).

perhaps you can fix both ?

It's an interesting point. The problem is that I don't actually have this hardware. On the patch which I sent, it was obvious what the intent. My guess is that msg1.bssid.data[] should have 6 elements instead of 7, but I don't feel confident enough to sign off on that.

Let's fix this bug which is obvious and let someone who knows how to fix that other question address it.

regards,
dan carpenter

#3 lore raw

Am 22.02.2012 08:54, schrieb Dan Carpenter:

Quoted 11 lines · walter harms
Quoted 9 lines

drivers/staging/wlan-ng/cfg80211.c int prism2_scan(struct wiphy *wiphy, struct net_device *dev, · line 360

Quoted 3 lines
drivers/staging/wlan-ng/cfg80211.c +1 -1
-	memset(&(msg1.bssid.data), 0xFF, sizeof(p80211item_pstr6_t));
+	memset(&msg1.bssid.data, 0xFF, sizeof(msg1.bssid.data));
 	msg1.bssid.data.len = 6;

maybe msg1.bssid.data.len is related to msg1.bssid.data ? I guess sizeof(msg1.bssid.data)-1 (why -1).

perhaps you can fix both ?

Quoted 9 lines

It's an interesting point. The problem is that I don't actually have this hardware. On the patch which I sent, it was obvious what the intent. My guess is that msg1.bssid.data[] should have 6 elements instead of 7, but I don't feel confident enough to sign off on that.

Let's fix this bug which is obvious and let someone who knows how to fix that other question address it.

Now it lokks better than before, lets wait what the maintainer can say about this. otherwise what about a /* FIXME: */ ?

re,
wh

#4 lore raw
Quoted 18 lines · On Wed, 22 Feb 2012 09:08:25 +0100 walter harms <[email protected]> wrote:

Am 22.02.2012 08:54, schrieb Dan Carpenter:

Quoted 17 lines · walter harms
Quoted 10 lines

drivers/staging/wlan-ng/cfg80211.c int prism2_scan(struct wiphy *wiphy, struct net_device *dev, · line 360

Quoted 4 lines
(quoted diff) +0 -1
-	memset(&(msg1.bssid.data), 0xFF,

sizeof(p80211item_pstr6_t));

drivers/staging/wlan-ng/cfg80211.c +1 -0
+	memset(&msg1.bssid.data, 0xFF, sizeof(msg1.bssid.data));
 	msg1.bssid.data.len = 6;

maybe msg1.bssid.data.len is related to msg1.bssid.data ? I guess sizeof(msg1.bssid.data)-1 (why -1).

perhaps you can fix both ?

It's an interesting point. The problem is that I don't actually have this hardware. On the patch which I sent, it was obvious what the intent. My guess is that msg1.bssid.data[] should have 6 elements instead of 7, but I don't feel confident enough to sign off on that.

msg1.bssid.data.data has 6 elements. msg1.bssid.data is a Pascal string, i.e. a length byte and 6 bytes of data.

The intention of the code must have been:

memset(&msg1.bssid.data.data, 0xFF, sizeof(msg1.bssid.data.data));

sizeof(msg1.bssid.data.data) is 6.

Writing 15 bytes to a structure that is 7 bytes long is certainly wrong and should be fixed.

I have the hardware, so please copy me if testing is needed.

Signature · 2 lines
Regards,
Pavel Roskin