[patch] Staging: wlan-ng: memsetting the wrong amount of data

[patch] Staging: wlan-ng: memsetting the wrong amount of data

[Dan Carpenter]

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 <dan.carpenter@oracle.com>

diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
index fb466f4..d518b31 100644
--- a/drivers/staging/wlan-ng/cfg80211.c
+++ b/drivers/staging/wlan-ng/cfg80211.c
@@ -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) {

Re: [patch] Staging: wlan-ng: memsetting the wrong amount of data

[walter harms]

Am 21.02.2012 15:18, schrieb Dan Carpenter:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
> index fb466f4..d518b31 100644
> --- a/drivers/staging/wlan-ng/cfg80211.c
> +++ b/drivers/staging/wlan-ng/cfg80211.c
> @@ -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

>  	if (request->n_ssids > 0) {
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [patch] Staging: wlan-ng: memsetting the wrong amount of data

[Dan Carpenter]

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

On Tue, Feb 21, 2012 at 05:39:42PM +0100, walter harms wrote:
> > -	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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] Staging: wlan-ng: memsetting the wrong amount of data

[walter harms]

Am 22.02.2012 08:54, schrieb Dan Carpenter:
> On Tue, Feb 21, 2012 at 05:39:42PM +0100, walter harms wrote:
>>> -	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.
> 

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

re,
 wh

Re: [patch] Staging: wlan-ng: memsetting the wrong amount of data

[Pavel Roskin]

On Wed, 22 Feb 2012 09:08:25 +0100
walter harms <wharms@bfs.de> wrote:

> Am 22.02.2012 08:54, schrieb Dan Carpenter:
> > On Tue, Feb 21, 2012 at 05:39:42PM +0100, walter harms wrote:
> >>> -	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.

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.

-- 
Regards,
Pavel Roskin