* [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
@ 2009-12-12 20:47 Daniel Mack
2009-12-15 9:43 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Daniel Mack @ 2009-12-12 20:47 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Mack, Dan Williams, Michael Hirsch, netdev, libertas-dev,
stable
We've experienced a long standing bug when quickly switching from
ad-hoc to managed mode on a hardware using a Libertas chipset.
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-12 20:47 [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs Daniel Mack
@ 2009-12-15 9:43 ` David Miller
2009-12-15 10:03 ` Johannes Berg
2009-12-15 10:16 ` Albert Cahalan
2009-12-15 16:29 ` Marcel Holtmann
2 siblings, 1 reply; 17+ messages in thread
From: David Miller @ 2009-12-15 9:43 UTC (permalink / raw)
To: daniel
Cc: linux-kernel, dcbw, m.hirsch, netdev, libertas-dev, stable,
linux-wireless
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/
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-15 9:43 ` David Miller
@ 2009-12-15 10:03 ` Johannes Berg
2009-12-15 10:05 ` Johannes Berg
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Johannes Berg @ 2009-12-15 10:03 UTC (permalink / raw)
To: David Miller
Cc: daniel, linux-kernel, dcbw, m.hirsch, netdev, libertas-dev,
stable, linux-wireless
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-15 10:03 ` Johannes Berg
@ 2009-12-15 10:05 ` Johannes Berg
2009-12-15 10:07 ` Johannes Berg
2009-12-16 3:58 ` Daniel Mack
2 siblings, 0 replies; 17+ messages in thread
From: Johannes Berg @ 2009-12-15 10:05 UTC (permalink / raw)
To: David Miller
Cc: daniel, linux-kernel, dcbw, m.hirsch, netdev, libertas-dev,
stable, linux-wireless
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-15 10:03 ` Johannes Berg
2009-12-15 10:05 ` Johannes Berg
@ 2009-12-15 10:07 ` Johannes Berg
[not found] ` <1260871634.3692.6.camel-YfaajirXv2244ywRPIzf9A@public.gmane.org>
2009-12-16 3:58 ` Daniel Mack
2 siblings, 1 reply; 17+ messages in thread
From: Johannes Berg @ 2009-12-15 10:07 UTC (permalink / raw)
To: David Miller
Cc: daniel, linux-kernel, dcbw, m.hirsch, netdev, libertas-dev,
stable, linux-wireless
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-15 10:03 ` Johannes Berg
2009-12-15 10:05 ` Johannes Berg
2009-12-15 10:07 ` Johannes Berg
@ 2009-12-16 3:58 ` Daniel Mack
2009-12-16 8:20 ` Johannes Berg
2 siblings, 1 reply; 17+ messages in thread
From: Daniel Mack @ 2009-12-16 3:58 UTC (permalink / raw)
To: Johannes Berg
Cc: David Miller, linux-kernel, dcbw, m.hirsch, netdev, libertas-dev,
stable, linux-wireless
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-16 3:58 ` Daniel Mack
@ 2009-12-16 8:20 ` Johannes Berg
0 siblings, 0 replies; 17+ messages in thread
From: Johannes Berg @ 2009-12-16 8:20 UTC (permalink / raw)
To: Daniel Mack
Cc: David Miller, linux-kernel, dcbw, m.hirsch, netdev, libertas-dev,
stable, linux-wireless
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-12 20:47 [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs Daniel Mack
2009-12-15 9:43 ` David Miller
@ 2009-12-15 10:16 ` Albert Cahalan
2009-12-15 16:29 ` Marcel Holtmann
2 siblings, 0 replies; 17+ messages in thread
From: Albert Cahalan @ 2009-12-15 10:16 UTC (permalink / raw)
To: Daniel Mack
Cc: linux-kernel, Michael Hirsch, libertas-dev, Dan Williams, netdev,
stable
On Sat, Dec 12, 2009 at 3:47 PM, Daniel Mack <daniel@caiaq.de> wrote:
> 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.
This is the wrong fix.
These are not strings. They are 32 arbitrary bytes. It is perfectly
legitimate to have a NUL byte in the middle; the use of C string
functions will corrupt the data.
For your testing I suggest:
a. start the SSID with '-'
b. include "/../" in the SSID
c. include UTF-16 surrogates wrongly encoded as UTF-8
d. include "\r\n" in the SSID
e. include quote and backslash characters in the SSID
f. include bytes in the 0x80 to 0x9f range, surrounded by ASCII
g. include bytes in the 0xc0 to 0xff range, surrounded by ASCII
h. include the sequence 0xc0,0x80 (Java UTF-8 pseudo-NUL)
i. include the NUL byte
j. end the SSID with a plain ASCII letter
Verify that the whole stack, from driver to GUI, can handle this.
That includes config files, command lines, /proc and /sys, etc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs
2009-12-12 20:47 [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs Daniel Mack
2009-12-15 9:43 ` David Miller
2009-12-15 10:16 ` Albert Cahalan
@ 2009-12-15 16:29 ` Marcel Holtmann
2 siblings, 0 replies; 17+ messages in thread
From: Marcel Holtmann @ 2009-12-15 16:29 UTC (permalink / raw)
To: Daniel Mack
Cc: linux-kernel, Dan Williams, Michael Hirsch, netdev, libertas-dev,
stable
Hi Daniel,
> We've experienced a long standing bug when quickly switching from
> ad-hoc to managed mode on a hardware using a Libertas chipset.
I don't know where you get your list of mailing lists from, but not
sending it to linux-wireless@vger.kernel.org will not really get you
anywhere. Check MAINTAINERS file first since it has all information
about it.
NETWORKING [WIRELESS]
M: "John W. Linville" <linville@tuxdriver.com>
L: linux-wireless@vger.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git
S: Maintained
F: net/mac80211/
F: net/rfkill/
F: net/wireless/
F: include/net/ieee80211*
F: include/linux/wireless.h
F: drivers/net/wireless/
Regards
Marcel
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2009-12-16 8:27 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-12-12 20:47 [PATCH] wireless: wext: allocate space for NULL-termination for 32byte SSIDs Daniel Mack
2009-12-15 9:43 ` David Miller
2009-12-15 10:03 ` Johannes Berg
2009-12-15 10:05 ` Johannes Berg
2009-12-15 10:07 ` Johannes Berg
[not found] ` <1260871634.3692.6.camel-YfaajirXv2244ywRPIzf9A@public.gmane.org>
2009-12-15 10:20 ` Daniel Mack
[not found] ` <20091215102035.GJ28375-ahpEBR4enfnCULTFXS99ULNAH6kLmebB@public.gmane.org>
2009-12-15 10:31 ` Johannes Berg
2009-12-15 10:37 ` Daniel Mack
2009-12-15 10:30 ` Holger Schurig
[not found] ` <200912151130.59103.holgerschurig-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2009-12-15 10:35 ` Johannes Berg
2009-12-16 6:54 ` Albert Cahalan
2009-12-16 8:19 ` Johannes Berg
2009-12-16 8:26 ` Holger Schurig
2009-12-16 3:58 ` Daniel Mack
2009-12-16 8:20 ` Johannes Berg
2009-12-15 10:16 ` Albert Cahalan
2009-12-15 16:29 ` Marcel Holtmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).