Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[David Miller]

From: Daniel Mack <daniel@caiaq.de>
Date: Sun, 13 Dec 2009 04:47:30 +0800

> We've experienced a long standing bug when quickly switching from
> ad-hoc to managed mode on a hardware using a Libertas chipset.

Can you please CC: linux-wireless for wireless patches?

Thanks.

> The effect is that after a number of mode transistions (sometimes as few
> as two sufficed), the kernel will oops at very strange locations, mostly
> in something like __kmem_alloc().
> 
> While the root cause turned out to be an issue with the wpa-supplicant
> which feeds the kernel driver with garbage, this occasion pointed out a
> bug in the wireless wext core when SSIDs with 32 byte lengths are passed
> from userspace. In this case, the string is not properly NULL-terminated
> which causes some other part to corrupt memory.
> 
> (In the particular case I observed, an SIOCSIWESSID was issued with
>  bogus data in iwp->pointer but iwp->length=32).
> 
> I admitedly couldn't find where the actual corruption itself happens,
> but with this trivial fix, I can't reproduce the bug anymore.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Dan Williams <dcbw@redhat.com>
> Cc: Michael Hirsch <m.hirsch@raumfeld.com>
> Cc: netdev@vger.kernel.org
> Cc: libertas-dev@lists.infradead.org
> Cc: stable@kernel.org
> ---
>  net/wireless/wext.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> ---
>  net/wireless/wext-core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
> index 5e1656b..3d8f4b0 100644
> --- a/net/wireless/wext-core.c
> +++ b/net/wireless/wext-core.c
> @@ -759,8 +759,8 @@ static int ioctl_standard_iw_point(struct iw_point *iwp, unsigned int cmd,
>  		}
>  	}
>  
> -	/* kzalloc() ensures NULL-termination for essid_compat. */
> -	extra = kzalloc(extra_size, GFP_KERNEL);
> +	/* kzalloc() +1 ensures NULL-termination for essid_compat. */
> +	extra = kzalloc(extra_size + 1, GFP_KERNEL);
>  	if (!extra)
>  		return -ENOMEM;
>  
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Johannes Berg]

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

On Tue, 2009-12-15 at 01:43 -0800, David Miller wrote:

> > The effect is that after a number of mode transistions (sometimes as few
> > as two sufficed), the kernel will oops at very strange locations, mostly
> > in something like __kmem_alloc().
> > 
> > While the root cause turned out to be an issue with the wpa-supplicant
> > which feeds the kernel driver with garbage, this occasion pointed out a
> > bug in the wireless wext core when SSIDs with 32 byte lengths are passed
> > from userspace. In this case, the string is not properly NULL-terminated
> > which causes some other part to corrupt memory.
> > 
> > (In the particular case I observed, an SIOCSIWESSID was issued with
> >  bogus data in iwp->pointer but iwp->length=32).
> > 
> > I admitedly couldn't find where the actual corruption itself happens,
> > but with this trivial fix, I can't reproduce the bug anymore.

Well you should try harder :)

> > -	/* kzalloc() ensures NULL-termination for essid_compat. */
> > -	extra = kzalloc(extra_size, GFP_KERNEL);
> > +	/* kzalloc() +1 ensures NULL-termination for essid_compat. */
> > +	extra = kzalloc(extra_size + 1, GFP_KERNEL);

That doesn't seem correct.

If this is used in a SET, then it is purely an in-kernel thing and
everything in the kernel is passed the length + data, and the kernel
MUST NEVER treat the SSID as a NUL-terminated string.

If this is used in a GET, then it will be filled up to 32 bytes by the
get handler, and the trailing \0 your patch reserves will never be
copied into userspace.

Since you indicate the kernel crashed, it is likely that libertas is
treating the buffer as a string, instead of an SSID.

That doesn't, however, make your fix and more valid. Whatever code uses
it as a string is clearly at fault, since this is a valid four-byte
SSID: "\x00\x01\x02\x03"

johannes

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

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Johannes Berg]

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

On Tue, 2009-12-15 at 11:03 +0100, Johannes Berg wrote:
> On Tue, 2009-12-15 at 01:43 -0800, David Miller wrote:
> 
> > > The effect is that after a number of mode transistions (sometimes as few
> > > as two sufficed), the kernel will oops at very strange locations, mostly
> > > in something like __kmem_alloc().
> > > 
> > > While the root cause turned out to be an issue with the wpa-supplicant
> > > which feeds the kernel driver with garbage, this occasion pointed out a
> > > bug in the wireless wext core when SSIDs with 32 byte lengths are passed
> > > from userspace. In this case, the string is not properly NULL-terminated
> > > which causes some other part to corrupt memory.

And, I forgot to mention, this is in fact not an issue or the "root
cause" of any issues -- it's completely intentional that wpa_supplicant
feeds the kernel with a random, valid, 32-byte SSID.

johannes

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

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Johannes Berg]

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

On Tue, 2009-12-15 at 11:03 +0100, Johannes Berg wrote:

> Since you indicate the kernel crashed, it is likely that libertas is
> treating the buffer as a string, instead of an SSID.

drivers/net/wireless/libertas$ grep lbs_deb_ * | grep ssid|grep '%s'
assoc.c:	lbs_deb_join("current SSID '%s', ssid length %u\n",
assoc.c:	lbs_deb_join("requested ssid '%s', ssid length %u\n",
assoc.c:	lbs_deb_join("ADHOC_START: SSID '%s', ssid length %u\n",
scan.c:		lbs_deb_wext("set_scan, essid '%s'\n",

johannes

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

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Daniel Mack]

On Tue, Dec 15, 2009 at 11:07:14AM +0100, Johannes Berg wrote:
> On Tue, 2009-12-15 at 11:03 +0100, Johannes Berg wrote:
> 
> > Since you indicate the kernel crashed, it is likely that libertas is
> > treating the buffer as a string, instead of an SSID.

Ok, I dodn't know that - thanks for pointing it out. In my setup here,
SSIDs are always valid alphanumeric strings, and I was mislead by the
comment that mentions the NULL-termination.

> drivers/net/wireless/libertas$ grep lbs_deb_ * | grep ssid|grep '%s'
> assoc.c:	lbs_deb_join("current SSID '%s', ssid length %u\n",
> assoc.c:	lbs_deb_join("requested ssid '%s', ssid length %u\n",
> assoc.c:	lbs_deb_join("ADHOC_START: SSID '%s', ssid length %u\n",
> scan.c:		lbs_deb_wext("set_scan, essid '%s'\n",

Those macros are stubbed out as nops in my setup, so they can
unfortunately not be the reason. I'll dig deeper :)

Thanks,
Daniel

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Johannes Berg]

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

On Tue, 2009-12-15 at 18:20 +0800, Daniel Mack wrote:

> > > Since you indicate the kernel crashed, it is likely that libertas is
> > > treating the buffer as a string, instead of an SSID.
> 
> Ok, I dodn't know that - thanks for pointing it out. In my setup here,
> SSIDs are always valid alphanumeric strings, and I was mislead by the
> comment that mentions the NULL-termination.

Yeah ... that's an unfortunate historic mistake in wireless extensions
that we try to fix up there. It cannot always be successfully fixed up,
but the NUL-termination is nothing that anything but this code should be
worried about or rely on.

> > drivers/net/wireless/libertas$ grep lbs_deb_ * | grep ssid|grep '%s'
> > assoc.c:	lbs_deb_join("current SSID '%s', ssid length %u\n",
> > assoc.c:	lbs_deb_join("requested ssid '%s', ssid length %u\n",
> > assoc.c:	lbs_deb_join("ADHOC_START: SSID '%s', ssid length %u\n",
> > scan.c:		lbs_deb_wext("set_scan, essid '%s'\n",
> 
> Those macros are stubbed out as nops in my setup, so they can
> unfortunately not be the reason. I'll dig deeper :)

Well, the stack trace ought to help.

johannes

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

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Daniel Mack]

On Tue, Dec 15, 2009 at 11:31:23AM +0100, Johannes Berg wrote:
> On Tue, 2009-12-15 at 18:20 +0800, Daniel Mack wrote:
> 
> > > drivers/net/wireless/libertas$ grep lbs_deb_ * | grep ssid|grep '%s'
> > > assoc.c:	lbs_deb_join("current SSID '%s', ssid length %u\n",
> > > assoc.c:	lbs_deb_join("requested ssid '%s', ssid length %u\n",
> > > assoc.c:	lbs_deb_join("ADHOC_START: SSID '%s', ssid length %u\n",
> > > scan.c:		lbs_deb_wext("set_scan, essid '%s'\n",
> > 
> > Those macros are stubbed out as nops in my setup, so they can
> > unfortunately not be the reason. I'll dig deeper :)
> 
> Well, the stack trace ought to help.

It unfortunately doesn't. The site that causes the problem does not
crash itself. It just corrupts some memory, and the actual crash happens
much later, which makes it so evil.

Daniel

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Holger Schurig]

> drivers/net/wireless/libertas$ grep lbs_deb_ * | grep ssid
> |grep '%s' 
> assoc.c:	lbs_deb_join("current SSID '%s', ssid length %u\n",
> assoc.c:	lbs_deb_join("requested ssid '%s', ssid length %u\n",
> assoc.c:	lbs_deb_join("ADHOC_START: SSID '%s', ssid
>               length %u\n", 
> scan.c:		lbs_deb_wext("set_scan, essid '%s'\n",

All those lines are gone once my cfg80211 lands.

Do you know any way to make sparse moan about them?


BTW: the libertas firmware sometimes treat an SSID as a 
zero-terminated string. There are some firmware commands that 
accept just an u8[32] bytes for the SSID, but not an ssid_len, 
e.g. in the CMD_802_11_AD_HOC_START command.

You therefore can't connect to the otherwise legitimate SSID of 
TEST\0\0\0.

-- 
http://www.holgerschurig.de

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Johannes Berg]

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

On Tue, 2009-12-15 at 11:30 +0100, Holger Schurig wrote:
> > drivers/net/wireless/libertas$ grep lbs_deb_ * | grep ssid
> > |grep '%s' 
> > assoc.c:	lbs_deb_join("current SSID '%s', ssid length %u\n",
> > assoc.c:	lbs_deb_join("requested ssid '%s', ssid length %u\n",
> > assoc.c:	lbs_deb_join("ADHOC_START: SSID '%s', ssid
> >               length %u\n", 
> > scan.c:		lbs_deb_wext("set_scan, essid '%s'\n",
> 
> All those lines are gone once my cfg80211 lands.
> 
> Do you know any way to make sparse moan about them?

Sorry, no, I don't think that's even possible unless you play dirty with
tricks like __iomem uses for instance but that'd require a lot of
casting in otherwise valid uses.

> BTW: the libertas firmware sometimes treat an SSID as a 
> zero-terminated string. There are some firmware commands that 
> accept just an u8[32] bytes for the SSID, but not an ssid_len, 
> e.g. in the CMD_802_11_AD_HOC_START command.
> 
> You therefore can't connect to the otherwise legitimate SSID of 
> TEST\0\0\0.

Ick! I guess your cfg80211 IBSS join handler needs to check for that
then and refuse such an SSID.

johannes


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

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Albert Cahalan]

On Tue, Dec 15, 2009 at 5:35 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Tue, 2009-12-15 at 11:30 +0100, Holger Schurig wrote:
>> > drivers/net/wireless/libertas$ grep lbs_deb_ * | grep ssid
>> > |grep '%s'
>> > assoc.c:    lbs_deb_join("current SSID '%s', ssid length %u\n",
>> > assoc.c:    lbs_deb_join("requested ssid '%s', ssid length %u\n",
>> > assoc.c:    lbs_deb_join("ADHOC_START: SSID '%s', ssid
>> >               length %u\n",
>> > scan.c:             lbs_deb_wext("set_scan, essid '%s'\n",
>>
>> All those lines are gone once my cfg80211 lands.
>>
>> Do you know any way to make sparse moan about them?
>
> Sorry, no, I don't think that's even possible unless you play dirty with
> tricks like __iomem uses for instance but that'd require a lot of
> casting in otherwise valid uses.
>
>> BTW: the libertas firmware sometimes treat an SSID as a
>> zero-terminated string. There are some firmware commands that
>> accept just an u8[32] bytes for the SSID, but not an ssid_len,
>> e.g. in the CMD_802_11_AD_HOC_START command.
>>
>> You therefore can't connect to the otherwise legitimate SSID of
>> TEST\0\0\0.
>
> Ick! I guess your cfg80211 IBSS join handler needs to check for that
> then and refuse such an SSID.

No, pad the SSID out to 32 bytes and let the firmware try.

First of all, isn't TEST\0\0\0 simply the wrong length anyway?
(that is, a length other than 32 is nonsense AFAIK)

Second of all, even if that is valid, the firmware probably handles
at least one SSID that starts with TEST and has some number
of NUL bytes on the end. Since you can't tell what that would be
with a particular firmware version, you might as well just let the
firmware try. The worst case failure here is that there is more than
one SSID of this form and you connect to the wrong one. If you
have a problem with this kind of trouble then you need ethernet.

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Johannes Berg]

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

On Wed, 2009-12-16 at 01:54 -0500, Albert Cahalan wrote:

> >> You therefore can't connect to the otherwise legitimate SSID of
> >> TEST\0\0\0.
> >
> > Ick! I guess your cfg80211 IBSS join handler needs to check for that
> > then and refuse such an SSID.
> 
> No, pad the SSID out to 32 bytes and let the firmware try.

No, if we _know_ the firmware will try to connect to "TEST" instead of
"TEST\0\0\0" then refusing it is the right thing to do.

> First of all, isn't TEST\0\0\0 simply the wrong length anyway?
> (that is, a length other than 32 is nonsense AFAIK)

No.

> Second of all, even if that is valid, the firmware probably handles
> at least one SSID that starts with TEST and has some number
> of NUL bytes on the end. Since you can't tell what that would be
> with a particular firmware version, you might as well just let the
> firmware try. The worst case failure here is that there is more than
> one SSID of this form and you connect to the wrong one. If you
> have a problem with this kind of trouble then you need ethernet.

No. An SSID is a uniquely defined, 1-32 byte long byte bit pattern. It
doesn't treat \0 special in any way as your comments suggest. If the
firmware stops matching at \0, the firmware is broken and shouldn't be
given a choice.

johannes

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

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Holger Schurig]

> First of all, isn't TEST\0\0\0 simply the wrong length anyway?
> (that is, a length other than 32 is nonsense AFAIK)

No, the SSID IE in a beacon encodes also a length. So the beacons 
from SSID of TEST, TEST\0 and TEST\0\0\0 are different.

This is because in the beacon, the SSID is *NOT* an u8[32], but 
an IE, which is struct { u8 type, u8 length, u8 data[] };

-- 
http://www.holgerschurig.de

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Daniel Mack]

On Tue, Dec 15, 2009 at 11:03:31AM +0100, Johannes Berg wrote:
> 
> > > -	/* kzalloc() ensures NULL-termination for essid_compat. */
> > > -	extra = kzalloc(extra_size, GFP_KERNEL);
> > > +	/* kzalloc() +1 ensures NULL-termination for essid_compat. */
> > > +	extra = kzalloc(extra_size + 1, GFP_KERNEL);
> 
> That doesn't seem correct.
> 
> If this is used in a SET, then it is purely an in-kernel thing and
> everything in the kernel is passed the length + data, and the kernel
> MUST NEVER treat the SSID as a NUL-terminated string.
> 
> If this is used in a GET, then it will be filled up to 32 bytes by the
> get handler, and the trailing \0 your patch reserves will never be
> copied into userspace.

The problem is the GET case. The libertas driver copies ssid_len
characters here and appends a trailing \0, which my patch caught now and
which caused memory corruption in before.

>From what I've seen, libertas _does_ treat the extra data correctly
at all places, I checked it several times now. (Btw, the %s format
string you pointed out all use print_ssid() to properly escape all
non-printable characters, so they're rules out, too).

I'll send a patch to fix the flaw in libertas.

Thanks,
Daniel

Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs

[Johannes Berg]

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

On Wed, 2009-12-16 at 11:58 +0800, Daniel Mack wrote:

> > If this is used in a GET, then it will be filled up to 32 bytes by the
> > get handler, and the trailing \0 your patch reserves will never be
> > copied into userspace.
> 
> The problem is the GET case. The libertas driver copies ssid_len
> characters here and appends a trailing \0, which my patch caught now and
> which caused memory corruption in before.
> 
> From what I've seen, libertas _does_ treat the extra data correctly
> at all places, I checked it several times now. (Btw, the %s format
> string you pointed out all use print_ssid() to properly escape all
> non-printable characters, so they're rules out, too).

Oh, ok, print_ssid() is correct of course, it gets the length.

> I'll send a patch to fix the flaw in libertas.

Thanks.

johannes

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