wireless extensions vs. 64-bit architectures

wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

Hi,

Wtf! After struggling with some strange problems with zd1211rw (see some
other mail) I decided to think again about what could possibly cause all
the other problems I'm having with it. The kernel seems fine, but iw*
userspace continually segfaults! And it also seems to be not
reproducible for most other people, I'd asked on IRC once a while.

Well. Some thinking and stracing and thinking later it occurred to me...
Hell! wext is ioctls and includes this gem:

struct  iw_point
{
  void __user   *pointer;       /* Pointer to the data  (in user space) */
  __u16         length;         /* number of fields or size in bytes */
  __u16         flags;          /* Optional params */
};

Of course nobody ever tells you this, but it's used in a shitload of
places.

Btw, did I mention that I'm running a stock debian powerpc 32-bit
userspace on my 64-bit machine. Oh and of course wext doesn't have any
32-in-64 compat code.

/me laughes manically about wext.

And don't tell me the fix is to use the netlink interface to wext.
Actually, I think it may have the same bug, it seems to be operating
with iw_point (or at least its size) too but I can't really tell, the
code's just too clear, I always just see right through it... Oh and I
still insist on removing the whole pile of junk, netlink interface
first.

Isn't there any possibility that we can kill userspace interfaces that
are terminally broken without keeping them for years to come?

Sorry. This is just too frustrating.

johannes
-- 
Now playing: Nightwish (Century Child) - End Of All Hope


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

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Tue, 2007-03-06 at 02:27 +0100, Johannes Berg wrote:
> Actually, I think it may have the same bug, it seems to be operating
> with iw_point (or at least its size) too

I'm told that the code that uses it is only internal and the size isn't
part of the userspace interface which makes this wrong. But we still
have many programs relying on ioctls and none relying on the wext/nl
interface so that isn't really a concern anyway.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Tue, Mar 06, 2007 at 02:27:26AM +0100, Johannes Berg wrote:
> Hi,
> 
> Wtf! After struggling with some strange problems with zd1211rw (see some
> other mail) I decided to think again about what could possibly cause all
> the other problems I'm having with it. The kernel seems fine, but iw*
> userspace continually segfaults! And it also seems to be not
> reproducible for most other people, I'd asked on IRC once a while.
> 
> Well. Some thinking and stracing and thinking later it occurred to me...
> Hell! wext is ioctls and includes this gem:
> 
> struct  iw_point
> {
>   void __user   *pointer;       /* Pointer to the data  (in user space) */
>   __u16         length;         /* number of fields or size in bytes */
>   __u16         flags;          /* Optional params */
> };
> 
> Of course nobody ever tells you this, but it's used in a shitload of
> places.

	Yep, and it's even in fs/compat_ioctl.c. Hint, hint ;-)

> Btw, did I mention that I'm running a stock debian powerpc 32-bit
> userspace on my 64-bit machine. Oh and of course wext doesn't have any
> 32-in-64 compat code.

	Please check again, it does.

> /me laughes manically about wext.
> 
> And don't tell me the fix is to use the netlink interface to wext.
> Actually, I think it may have the same bug, it seems to be operating
> with iw_point (or at least its size) too but I can't really tell, the
> code's just too clear, I always just see right through it... Oh and I
> still insist on removing the whole pile of junk, netlink interface
> first.

	Well, why don't you go and check it. For example, check
where IW_EV_POINT_OFF is used.

> Isn't there any possibility that we can kill userspace interfaces that
> are terminally broken without keeping them for years to come?

	Well, is there a possibility that people check the facts
before making bold assumptions ?

> Sorry. This is just too frustrating.

	Yes, you are perfectly right. This continuous bashing of wext
for no good reason is too frustrating.

> johannes

	Now, back to the problem. You seem to have an intermitent
crash. If the stuff above was broken, it would systematically crash,
because it would always get stuff at an offset.
	The fact that the crash is not systematic leads me to believe
that something else is at play, such as a compiler optimisation gone
bad, some memory condition, or a driver returning corrupted data to
wext and iwconfig not checking bad data properly.
	If you were to give me a proper bug report, there is a chance
that we might make progress.

	Have fun...

	Jean

Re: wireless extensions vs. 64-bit architectures

[Michael Buesch]

On Tuesday 06 March 2007 18:13, Jean Tourrilhes wrote:
> On Tue, Mar 06, 2007 at 02:27:26AM +0100, Johannes Berg wrote:
> > Hi,
> > 
> > Wtf! After struggling with some strange problems with zd1211rw (see some
> > other mail) I decided to think again about what could possibly cause all
> > the other problems I'm having with it. The kernel seems fine, but iw*
> > userspace continually segfaults! And it also seems to be not
> > reproducible for most other people, I'd asked on IRC once a while.
> > 
> > Well. Some thinking and stracing and thinking later it occurred to me...
> > Hell! wext is ioctls and includes this gem:
> > 
> > struct  iw_point
> > {
> >   void __user   *pointer;       /* Pointer to the data  (in user space) */
> >   __u16         length;         /* number of fields or size in bytes */
> >   __u16         flags;          /* Optional params */
> > };
> > 
> > Of course nobody ever tells you this, but it's used in a shitload of
> > places.
> 
> 	Yep, and it's even in fs/compat_ioctl.c. Hint, hint ;-)

Ok, it is wrapping the following ioctls:

HANDLE_IOCTL(SIOCGIWRANGE, do_wireless_ioctl)
HANDLE_IOCTL(SIOCSIWSPY, do_wireless_ioctl)
HANDLE_IOCTL(SIOCGIWSPY, do_wireless_ioctl)
HANDLE_IOCTL(SIOCSIWTHRSPY, do_wireless_ioctl)
HANDLE_IOCTL(SIOCGIWTHRSPY, do_wireless_ioctl)
HANDLE_IOCTL(SIOCGIWAPLIST, do_wireless_ioctl)
HANDLE_IOCTL(SIOCGIWSCAN, do_wireless_ioctl)
HANDLE_IOCTL(SIOCSIWESSID, do_wireless_ioctl)
HANDLE_IOCTL(SIOCGIWESSID, do_wireless_ioctl)
HANDLE_IOCTL(SIOCSIWNICKN, do_wireless_ioctl)
HANDLE_IOCTL(SIOCGIWNICKN, do_wireless_ioctl)
HANDLE_IOCTL(SIOCSIWENCODE, do_wireless_ioctl)
HANDLE_IOCTL(SIOCGIWENCODE, do_wireless_ioctl)

What about SIOCSIWSCAN, SIOCSIWENCODEEXT, SIOCGIWENCODEEXT
and some others that also use iw_point?

-- 
Greetings Michael.

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Tue, Mar 06, 2007 at 07:43:06PM +0100, Michael Buesch wrote:
> > 
> > 	Yep, and it's even in fs/compat_ioctl.c. Hint, hint ;-)
> 
> Ok, it is wrapping the following ioctls:
> 
> HANDLE_IOCTL(SIOCGIWRANGE, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWTHRSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWTHRSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWAPLIST, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWSCAN, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWESSID, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWESSID, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWNICKN, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWNICKN, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWENCODE, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWENCODE, do_wireless_ioctl)
> 
> What about SIOCSIWSCAN, SIOCSIWENCODEEXT, SIOCGIWENCODEEXT
> and some others that also use iw_point?

	Yep, good point.
	SIOCSIWSCAN is up there. I did not realise that all the WPA
ioctls are missing. That's easy enough to fix, remember that you have
the full description of the ioctls in wireless.c.
	I'll try to do a patch if I find 5 min, but feel free to
forward something to John L.
	Thanks a lot !

> Greetings Michael.

	Jean

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Tue, Mar 06, 2007 at 07:43:06PM +0100, Michael Buesch wrote:
> 
> Ok, it is wrapping the following ioctls:
> 
> HANDLE_IOCTL(SIOCGIWRANGE, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWTHRSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWTHRSPY, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWAPLIST, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWSCAN, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWESSID, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWESSID, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWNICKN, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWNICKN, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCSIWENCODE, do_wireless_ioctl)
> HANDLE_IOCTL(SIOCGIWENCODE, do_wireless_ioctl)
> 
> What about SIOCSIWSCAN, SIOCSIWENCODEEXT, SIOCGIWENCODEEXT
> and some others that also use iw_point?

	Ok, please check the patch attached. I don't have a box to
test that on, and on my 32 bit kernel it is not even compiled, but I
believe I got everything all right.
	Please push that to the usual channels...

> Greetings Michael.

	Thanks again,

	Jean

Signed-off-by: Jean Tourrilhes <jt@hpl.hp.com>

------------------------------------------------------------------

diff -u -p linux/fs/compat_ioctl.j1.c  linux/fs/compat_ioctl.c
--- linux/fs/compat_ioctl.j1.c	2007-03-06 17:49:33.000000000 -0800
+++ linux/fs/compat_ioctl.c	2007-03-06 17:56:19.000000000 -0800
@@ -2553,11 +2553,15 @@ HANDLE_IOCTL(I2C_RDWR, do_i2c_rdwr_ioctl
 HANDLE_IOCTL(I2C_SMBUS, do_i2c_smbus_ioctl)
 /* wireless */
 HANDLE_IOCTL(SIOCGIWRANGE, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCGIWPRIV, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCGIWSTATS, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCSIWSPY, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCGIWSPY, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCSIWTHRSPY, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCGIWTHRSPY, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCSIWMLME, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCGIWAPLIST, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCSIWSCAN, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCGIWSCAN, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCSIWESSID, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCGIWESSID, do_wireless_ioctl)
@@ -2565,6 +2569,11 @@ HANDLE_IOCTL(SIOCSIWNICKN, do_wireless_i
 HANDLE_IOCTL(SIOCGIWNICKN, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCSIWENCODE, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCGIWENCODE, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCSIWGENIE, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCGIWGENIE, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCSIWENCODEEXT, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCGIWENCODEEXT, do_wireless_ioctl)
+HANDLE_IOCTL(SIOCSIWPMKSA, do_wireless_ioctl)
 HANDLE_IOCTL(SIOCSIFBR, old_bridge_ioctl)
 HANDLE_IOCTL(SIOCGIFBR, old_bridge_ioctl)
 HANDLE_IOCTL(RTC_IRQP_READ32, rtc_ioctl)

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Tue, 2007-03-06 at 18:03 -0800, Jean Tourrilhes wrote:

> 	Ok, please check the patch attached. I don't have a box to
> test that on, and on my 32 bit kernel it is not even compiled, but I
> believe I got everything all right.

Don't I wish it was that easy... Granted, that does seem to fix the
issue with iw_point but that's not the only thing that's broken. Here's
the next thing, I think this only happens after applying that patch,
without it probably just craps out earlier. Same thing happens with
iwevent (and maybe other things too.)

Enabling debugging in wireless tools yields (on my 64-bit kernel with
32-bit userland):
# LD_LIBRARY_PATH=.  ./iwlist wlan0 scan
Scan result 4096
[00:18:8B:15:50:8A:35:E0:00:01:00:C0:02:E1:B8:0E:C0:00:00:00:50:8A:35:F0:00:19:8B:1B:50:8A:35:E0:00:09:00:01:50:8A:35:F0:47:6F:6C:6F:73:4E:65:74:7A:00:18:8B:01:50:8A:35:E0:49:45:45:45:20:38:30:32:2E:31:31:62:67:00:35:F0:00:0C:8B:07:50:8A:35:E0:00:00:00:03:00:10:8B:05:50:8A:35:E0:00:00:00:07:00:00:00:32:00:10:8B:2B:50:8A:35:E0:00:00:08:00:67:00:35:F0:00:70:8B:21:50:8A:35:E0:00:0F:42:40:00:00:00:32:00:1E:84:80:00:00:00:32:00:53:EC:60:00:00:00:32:00:5B:8D:80:00:00:00:32:00:89:54:40:00:00:00:32:00:A7:D8:C0:00:00:00:<lots more zeroes>
wlan0     Scan completed : 
DBG - stream->current = 0x10019008, stream->value = (nil), stream->end = 0x1001a008
DBG - iwe->cmd = 0x8B15, iwe->len = 24

                         ^^^^^^^^^^^^^ that's already bogus


DBG - event_type = 6, event_len = 16, pointer = 0x1001900c
          Cell 01 - Address: 35:E0:00:01:00:C0 
DBG - stream->current = 0x10019020, stream->value = (nil), stream->end = 0x1001a008
DBG - iwe->cmd = 0x8B1B, iwe->len = 25
DBG - event_type = 8, event_len = 4, pointer = 0x10019024 
DBG - extra_len = 17, token_len = 20618, token = 20618, max = 33, min = 0
                    ESSID:"" [224] 
DBG - stream->current = 0x10019039, stream->value = (nil), stream->end = 0x1001a008
DBG - iwe->cmd = 0x8B01, iwe->len = 24
DBG - event_type = 2, event_len = 16, pointer = 0x1001903d
                    Protocol:P�5�IEEE 802.11b 
DBG - stream->current = 0x10019051, stream->value = (nil), stream->end = 0x1001a008
DBG - iwe->cmd = 0x8B07, iwe->len = 12 
DBG - event_type = 4, event_len = 4, pointer = 0x10019055
Segmentation fault

Ooops.

Whereas on a 32-bit machine in the same environment it is as it should
be:

Scan result 264
[00:14:8B:15:00:01:00:C0:02:E1:B8:0E:C0:02:5A:F4:9F:CF:18:C0:00:11:8B:1B:00:09:00:01:47:6F:6C:6F:73:4E:65:74:7A:00:14:8B:01:49:45:45:45:20:38:30:32:2E:31:31:62:67:00:18:C0:00:08:8B:07:00:00:00:03:00:0C:8B:05:00:00:00:07:00:00:00:32:00:08:8B:2B:00:00:08:00:00:6C:8B:21:00:0F:42:40:00:00:08:00:00:1E:84:80:00:00:08:00:00:53:EC:60:00:00:08:00:00:5B:8D:80:00:00:08:00:00:89:54:40:00:00:08:00:00:A7:D8:C0:00:00:08:00:00:B7:1B:00:00:00:08:00:01:12:A8:80:00:00:08:00:01:4F:B1:80:00:00:08:00:01:6E:36:00:00:00:08:00:02:25:51:00:00:00:08:00:02:DC:6C:00:00:00:08:00:03:37:F9:80:00:00:08:00:00:08:8C:01:64:00:00:47:00:20:8C:05:00:18:00:00:DD:16:00:50:F2:01:01:00:00:50:F2:02:01:00:00:50:F2:02:01:00:00:50:F2:02:00:1F:8C:02:00:17:00:00:20:4C:61:73:74:20:62:65:61:63:6F:6E:3A:20:32:32:30:6D:73:20:61:67:6F]
eth2      Scan completed : 
DBG - stream->current = 0x10019008, stream->value = (nil), stream->end = 0x10019110
DBG - iwe->cmd = 0x8B15, iwe->len = 20 

                         ^^^^^^^^^^^^^ look, 4 bytes less. I wonder why that is!


DBG - event_type = 6, event_len = 16, pointer = 0x1001900c
          Cell 01 - Address: 00:C0:02:E1:B8:0E 
DBG - stream->current = 0x1001901c, stream->value = (nil), stream->end = 0x10019110
[and lots more skipped for clarity]


So there. Do you want to know why too?

Let's look at a debug info dump from my 64-bit kernel:

 <1><96de>: Abbrev Number: 33 (DW_TAG_structure_type)
  <96df>     DW_AT_sibling     : <9717>
  <96e3>     DW_AT_name        : (indirect string, offset: 0x4278): iw_event
  <96e7>     DW_AT_byte_size   : 24
  <96e8>     DW_AT_decl_file   : 225
  <96e9>     DW_AT_decl_line   : 1064
 <2><96eb>: Abbrev Number: 23 (DW_TAG_member)
  <96ec>     DW_AT_name        : len
  <96f0>     DW_AT_decl_file   : 225
  <96f1>     DW_AT_decl_line   : 1065
  <96f3>     DW_AT_type        : <ed>
  <96f7>     DW_AT_data_member_location: 2 byte block: 23 0     (DW_OP_plus_uconst: 0)
 <2><96fa>: Abbrev Number: 23 (DW_TAG_member)
  <96fb>     DW_AT_name        : cmd
  <96ff>     DW_AT_decl_file   : 225
  <9700>     DW_AT_decl_line   : 1066
  <9702>     DW_AT_type        : <ed>
  <9706>     DW_AT_data_member_location: 2 byte block: 23 2     (DW_OP_plus_uconst: 2)
 <2><9709>: Abbrev Number: 23 (DW_TAG_member)
  <970a>     DW_AT_name        : u
  <970c>     DW_AT_decl_file   : 225
  <970d>     DW_AT_decl_line   : 1067
  <970f>     DW_AT_type        : <956e>
  <9713>     DW_AT_data_member_location: 2 byte block: 23 8     (DW_OP_plus_uconst: 8)


The same structure from the 32-bit user space program (iwlist):

 <1><c3a>: Abbrev Number: 15 (DW_TAG_structure_type)
  <c3b>     DW_AT_sibling     : <c73>
  <c3f>     DW_AT_name        : (indirect string, offset: 0x32b): iw_event
  <c43>     DW_AT_byte_size   : 20
  <c44>     DW_AT_decl_file   : 96
  <c45>     DW_AT_decl_line   : 1095
 <2><c47>: Abbrev Number: 16 (DW_TAG_member)
  <c48>     DW_AT_name        : len
  <c4c>     DW_AT_decl_file   : 96
  <c4d>     DW_AT_decl_line   : 1096
  <c4f>     DW_AT_type        : <550>
  <c53>     DW_AT_data_member_location: 2 byte block: 23 0      (DW_OP_plus_uconst: 0)
 <2><c56>: Abbrev Number: 16 (DW_TAG_member)
  <c57>     DW_AT_name        : cmd
  <c5b>     DW_AT_decl_file   : 96
  <c5c>     DW_AT_decl_line   : 1097
  <c5e>     DW_AT_type        : <550>
  <c62>     DW_AT_data_member_location: 2 byte block: 23 2      (DW_OP_plus_uconst: 2)
 <2><c65>: Abbrev Number: 16 (DW_TAG_member)
  <c66>     DW_AT_name        : u
  <c68>     DW_AT_decl_file   : 96
  <c69>     DW_AT_decl_line   : 1098
  <c6b>     DW_AT_type        : <7e0>
  <c6f>     DW_AT_data_member_location: 2 byte block: 23 4      (DW_OP_plus_uconst: 4)

Compare the last lines of both (I'd have posted pahole output but the
dwarf lib it uses doesn't parse 64-bit binaries correctly when it is
compiled as a 32-bit binary.) 

Now look at the definition of IW_EV_LCP_LEN. Or wait, how about we look
at the comment in front of it:

  /* Size of the Event prefix (including padding and alignement junk) */


Now, I don't know what gcc for ia64 does and I don't have a cross
compiler to check, but on powerpc it does this. I think the reason for
this here is iw_point again since it is part of the union iwreq_data
which means that the whole union requires 8-byte alignment on 64-bit
architectures where it's part of struct iw_event.

The easiest "fix" would be to make the structure packed, but existing
64-bit userspace expects the struct with padding while existing 32-bit
userspace (which of course includes 32-bit userspace running on 64-bit
machines) expects no padding... So that "fix" breaks all 64-bit
userspace, great.

Oh, btw, this also means that we have an information leak on 64-bit
kernels. Those alignment bytes aren't ever cleared or anything, they
come right from the stack since most users of this just use a struct
iw_event on the stack which is then memcpy()ed right into the userspace
buffer. For example those bytes 5 through 8 ("50:8A:35:E0") in the first
buffer above. This is generally considered a security problem.

Have fun...

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 15:39 +0100, Johannes Berg wrote:

> Now, I don't know what gcc for ia64 does and I don't have a cross
> compiler to check, but on powerpc it does this.

As expected, the same happens on x86_64 (thanks to Michael Wu for the
debug dump):

 <1><e13>: Abbrev Number: 17 (DW_TAG_structure_type)
     DW_AT_sibling     : <e4c>
     DW_AT_name        : (indirect string, offset: 0x33d): iw_event
     DW_AT_byte_size   : 24
     DW_AT_decl_file   : 100
     DW_AT_decl_line   : 1049
 <2><e20>: Abbrev Number: 18 (DW_TAG_member)
     DW_AT_name        : len
     DW_AT_decl_file   : 100
     DW_AT_decl_line   : 1050
     DW_AT_type        : <58b>
     DW_AT_data_member_location: 2 byte block: 23 0     (DW_OP_plus_uconst: 0)
 <2><e2f>: Abbrev Number: 18 (DW_TAG_member)
     DW_AT_name        : cmd
     DW_AT_decl_file   : 100
     DW_AT_decl_line   : 1051
     DW_AT_type        : <58b>
     DW_AT_data_member_location: 2 byte block: 23 2     (DW_OP_plus_uconst: 2)
 <2><e3e>: Abbrev Number: 18 (DW_TAG_member)
     DW_AT_name        : u
     DW_AT_decl_file   : 100
     DW_AT_decl_line   : 1052
     DW_AT_type        : <9bf>
     DW_AT_data_member_location: 2 byte block: 23 8     (DW_OP_plus_uconst: 8)

Hence, on those machines 32-bit userspace is also broken and we have the
information leak too.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

On Thu, 2007-03-08 at 15:39 +0100, Johannes Berg wrote:

> Oh, btw, this also means that we have an information leak on 64-bit
> kernels. Those alignment bytes aren't ever cleared or anything, they
> come right from the stack since most users of this just use a struct
> iw_event on the stack which is then memcpy()ed right into the userspace
> buffer. For example those bytes 5 through 8 ("50:8A:35:E0") in the first
> buffer above. This is generally considered a security problem.

Patch below might fix it. If it does, please send it on to Linus (or
akpm), stable and whoever else might be interested in backporting it
(distros? or do they cherrypick from stable?)

I'm unable to test it as I'll be away from my only 64-bit machine for at
least until the 19th or 20th.

Yeah, I know sparse warns about this on 32-bit machines... But it seems
useless to try to fix that warning.

johannes

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Subject: fix information leak in wireless extensions (on 64-bit architectures)

On 64-bit architectures wireless extensions leak information from the
kernel stack due to padding inside structs not being cleared before they
are copied to userspace.

This is available to unprivileged users since they can listen for
wireless events and obtain scan results.

This patch fixes it by explicitly clearing the padding.

Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>

---
 include/net/iw_handler.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

--- wireless-dev.orig/include/net/iw_handler.h	2007-03-08 18:17:43.384642258 +0100
+++ wireless-dev/include/net/iw_handler.h	2007-03-08 18:28:12.704642258 +0100
@@ -484,6 +484,9 @@ iwe_stream_add_event(char *	stream,		/* 
 		     struct iw_event *iwe,	/* Payload */
 		     int	event_len)	/* Real size of payload */
 {
+	/* clear padding */
+	memset((char*)iwe + 4, 0, IW_EV_LCP_LEN - 4);
+
 	/* Check if it's possible */
 	if(likely((stream + event_len) < ends)) {
 		iwe->len = event_len;
@@ -505,6 +508,10 @@ iwe_stream_add_point(char *	stream,		/* 
 		     char *	extra)		/* More payload */
 {
 	int	event_len = IW_EV_POINT_LEN + iwe->u.data.length;
+
+	/* clear padding */
+	memset((char*)iwe + 4, 0, IW_EV_LCP_LEN - 4);
+
 	/* Check if it's possible */
 	if(likely((stream + event_len) < ends)) {
 		iwe->len = event_len;
@@ -531,6 +538,9 @@ iwe_stream_add_value(char *	event,		/* E
 		     struct iw_event *iwe,	/* Payload */
 		     int	event_len)	/* Real size of payload */
 {
+	/* clear padding */
+	memset((char*)iwe + 4, 0, IW_EV_LCP_LEN - 4);
+
 	/* Don't duplicate LCP */
 	event_len -= IW_EV_LCP_LEN;
 
@@ -558,6 +568,9 @@ iwe_stream_check_add_event(char *	stream
 			   int		event_len,	/* Size of payload */
 			   int *	perr)		/* Error report */
 {
+	/* clear padding */
+	memset((char*)iwe + 4, 0, IW_EV_LCP_LEN - 4);
+
 	/* Check if it's possible, set error if not */
 	if(likely((stream + event_len) < ends)) {
 		iwe->len = event_len;
@@ -582,6 +595,10 @@ iwe_stream_check_add_point(char *	stream
 			   int *	perr)		/* Error report */
 {
 	int	event_len = IW_EV_POINT_LEN + iwe->u.data.length;
+
+	/* clear padding */
+	memset((char*)iwe + 4, 0, IW_EV_LCP_LEN - 4);
+
 	/* Check if it's possible */
 	if(likely((stream + event_len) < ends)) {
 		iwe->len = event_len;
@@ -611,6 +628,9 @@ iwe_stream_check_add_value(char *	event,
 			   int		event_len,	/* Size of payload */
 			   int *	perr)		/* Error report */
 {
+	/* clear padding */
+	memset((char*)iwe + 4, 0, IW_EV_LCP_LEN - 4);
+
 	/* Don't duplicate LCP */
 	event_len -= IW_EV_LCP_LEN;
 

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Thu, Mar 08, 2007 at 03:39:07PM +0100, Johannes Berg wrote:
> On Tue, 2007-03-06 at 18:03 -0800, Jean Tourrilhes wrote:
> 
> > 	Ok, please check the patch attached. I don't have a box to
> > test that on, and on my 32 bit kernel it is not even compiled, but I
> > believe I got everything all right.
> 
> Don't I wish it was that easy... Granted, that does seem to fix the
> issue with iw_point but that's not the only thing that's broken. Here's
> the next thing, I think this only happens after applying that patch,
> without it probably just craps out earlier. Same thing happens with
> iwevent (and maybe other things too.)
> 
> Enabling debugging in wireless tools yields (on my 64-bit kernel with
> 32-bit userland):
> # LD_LIBRARY_PATH=.  ./iwlist wlan0 scan
> Scan result 4096
> [00:18:8B:15:50:8A:35:E0:00:01:00:C0:02:E1:B8:0E:C0:00:00:00:50:8A:35:F0:00:19:8B:1B:50:8A:35:E0:00:09:00:01:50:8A:35:F0:47:6F:6C:6F:73:4E:65:74:7A:00:18:8B:01:50:8A:35:E0:49:45:45:45:20:38:30:32:2E:31:31:62:67:00:35:F0:00:0C:8B:07:50:8A:35:E0:00:00:00:03:00:10:8B:05:50:8A:35:E0:00:00:00:07:00:00:00:32:00:10:8B:2B:50:8A:35:E0:00:00:08:00:67:00:35:F0:00:70:8B:21:50:8A:35:E0:00:0F:42:40:00:00:00:32:00:1E:84:80:00:00:00:32:00:53:EC:60:00:00:00:32:00:5B:8D:80:00:00:00:32:00:89:54:40:00:00:00:32:00:A7:D8:C0:00:00:00:<lots more zeroes>
> wlan0     Scan completed : 
> DBG - stream->current = 0x10019008, stream->value = (nil), stream->end = 0x1001a008
> DBG - iwe->cmd = 0x8B15, iwe->len = 24
> 
>                          ^^^^^^^^^^^^^ that's already bogus

	Ok, now I get it. It's an alignement issue, the kernel align
struct to 64 bit boundary, whereas the userspace expect it to 32 bit
boundary. Pretty nasty.
	A proper fix would involve forcing the alignement in the
kernel. Unfortunately, that would break 64bit->64bit configs. I think
I can build a workaround for this in iwlib.

	Thanks for the detailed bug report !

	Jean

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 10:49 -0800, Jean Tourrilhes wrote:

> 	A proper fix would involve forcing the alignement in the
> kernel. Unfortunately, that would break 64bit->64bit configs. I think
> I can build a workaround for this in iwlib.

Not easily I think. You'd have to get something that has a well-defined
result and see whether padding is present or not. The MAC address might
be good enough (due to len being 24 instead of the expected 20) though.
Thing is that it's really hard to figure out (even at runtime) whether
the kernel and machine are 64 or 32-bits.

I'd think this is a kernel bug and 32-bit userspace should rightfully be
able to expect 32-bit aligned structs, no? Actually fixing it in the
kernel would not be trivial though.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Thu, Mar 08, 2007 at 08:08:29PM +0100, Johannes Berg wrote:
> On Thu, 2007-03-08 at 10:49 -0800, Jean Tourrilhes wrote:
> 
> > 	A proper fix would involve forcing the alignement in the
> > kernel. Unfortunately, that would break 64bit->64bit configs. I think
> > I can build a workaround for this in iwlib.
> 
> Not easily I think. You'd have to get something that has a well-defined
> result and see whether padding is present or not. The MAC address might
> be good enough (due to len being 24 instead of the expected 20) though.
> Thing is that it's really hard to figure out (even at runtime) whether
> the kernel and machine are 64 or 32-bits.

	I'm looking into that. The good thing is that we have
redundant information, so we can check that things don't match. It's a
bit more complex because some of those take variable parameters.

> I'd think this is a kernel bug and 32-bit userspace should rightfully be
> able to expect 32-bit aligned structs, no? Actually fixing it in the
> kernel would not be trivial though.

	What we could do is have every 64 bit kernel return things on
a 32 bit boundary, irrespective of userspace used. That would break
current 64 bit userspace.

> johannes

	Thanks !

	Jean

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 11:13 -0800, Jean Tourrilhes wrote:

> 	I'm looking into that. The good thing is that we have
> redundant information, so we can check that things don't match. It's a
> bit more complex because some of those take variable parameters.

Yeah, and it also only happens with those event streams I think. But I
haven't checked other possible places.

> > I'd think this is a kernel bug and 32-bit userspace should rightfully be
> > able to expect 32-bit aligned structs, no? Actually fixing it in the
> > kernel would not be trivial though.
> 
> 	What we could do is have every 64 bit kernel return things on
> a 32 bit boundary, irrespective of userspace used. That would break
> current 64 bit userspace.

Yeah, the only way to fix the bug without breaking that would be to
return different structs for the different userspaces which sounds
really complex.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 10:49 -0800, Jean Tourrilhes wrote:
> 	A proper fix would involve forcing the alignement in the
> kernel. Unfortunately, that would break 64bit->64bit configs. I think
> I can build a workaround for this in iwlib.

Actually, other tools like network manager, wpa supplicant and possibly
more don't use iwlib afaik and are affected too, so the only real fix
seems to be possible in the kernel.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Jouni Malinen]

On Thu, Mar 08, 2007 at 08:27:22PM +0100, Johannes Berg wrote:
> On Thu, 2007-03-08 at 10:49 -0800, Jean Tourrilhes wrote:
> > 	A proper fix would involve forcing the alignement in the
> > kernel. Unfortunately, that would break 64bit->64bit configs. I think
> > I can build a workaround for this in iwlib.
> 
> Actually, other tools like network manager, wpa supplicant and possibly
> more don't use iwlib afaik and are affected too, so the only real fix
> seems to be possible in the kernel.

Yes, workaround in just iwlib is not enough. If the only possible
solution is user space workaround, it better be documented (and
communicated to maintainers of user space apps) well so that
all user space programs not using iwlib can be modified, too.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 11:34 -0800, Jouni Malinen wrote:

> Yes, workaround in just iwlib is not enough. If the only possible
> solution is user space workaround, it better be documented (and
> communicated to maintainers of user space apps) well so that
> all user space programs not using iwlib can be modified, too.

The more I think about it the worse it gets. Think about wireless events
where both 32 and 64-bit userspace programs may be listening... That
means we can't even fix it in the kernel without breaking something.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Thu, Mar 08, 2007 at 08:40:01PM +0100, Johannes Berg wrote:
> On Thu, 2007-03-08 at 11:34 -0800, Jouni Malinen wrote:
> 
> > Yes, workaround in just iwlib is not enough. If the only possible
> > solution is user space workaround, it better be documented (and
> > communicated to maintainers of user space apps) well so that
> > all user space programs not using iwlib can be modified, too.
> 
> The more I think about it the worse it gets. Think about wireless events
> where both 32 and 64-bit userspace programs may be listening... That
> means we can't even fix it in the kernel without breaking something.
> 
> johannes

	This is exactly what I was pointing out earlier. Well,
actually, there may be ways of fixing it in the kernel, but that would
be real ugly, and I don't want to go there.

	I've just released wireless_tools.29.pre15.tar.gz. This is
supposed to include a "band-aid" for that problem. To the best of my
knowledge, it should catch the problem and not introduce false
positive. I would be glad if you guys would have a quick look into it,
because obviously I can't test it.

	Now, about the way forward...
	First possiblity, we could stick with this band-aid
permanently.

	Second possiblity : we do the right thing and plan a API
change to return struct always aligned on 32 bits. This way, when we
get 128 bit processor, we don't have to add another band aid ;-)
	It would work like the ESSID changeover. We pick a WE version
changeover. We introduce userspace that can deal with before and
after. After 1 or 2 years, we flip the switch. After another 1 or 2
years, we get rid of backward compatibility.

	Third possibility : we declare 32 bit userspace on 64 bit
kernel as not supported and advise users to get a 64 bit
userspace. The number of bug report on that issue would suggest that
very few users are in this case.

	I know the userspace guys will hate (1) and hate even more (2).

	Regards,

	Jean

Re: wireless extensions vs. 64-bit architectures

[Randy Dunlap]

On Thu, 8 Mar 2007 14:11:28 -0800 Jean Tourrilhes wrote:

> On Thu, Mar 08, 2007 at 08:40:01PM +0100, Johannes Berg wrote:
> > On Thu, 2007-03-08 at 11:34 -0800, Jouni Malinen wrote:
> > 
> > > Yes, workaround in just iwlib is not enough. If the only possible
> > > solution is user space workaround, it better be documented (and
> > > communicated to maintainers of user space apps) well so that
> > > all user space programs not using iwlib can be modified, too.
> > 
> > The more I think about it the worse it gets. Think about wireless events
> > where both 32 and 64-bit userspace programs may be listening... That
> > means we can't even fix it in the kernel without breaking something.
> > 
> > johannes
> 
> 	This is exactly what I was pointing out earlier. Well,
> actually, there may be ways of fixing it in the kernel, but that would
> be real ugly, and I don't want to go there.
> 
> 	I've just released wireless_tools.29.pre15.tar.gz. This is
> supposed to include a "band-aid" for that problem. To the best of my
> knowledge, it should catch the problem and not introduce false
> positive. I would be glad if you guys would have a quick look into it,
> because obviously I can't test it.
> 
> 	Now, about the way forward...
> 	First possiblity, we could stick with this band-aid
> permanently.
> 
> 	Second possiblity : we do the right thing and plan a API
> change to return struct always aligned on 32 bits. This way, when we
> get 128 bit processor, we don't have to add another band aid ;-)
> 	It would work like the ESSID changeover. We pick a WE version
> changeover. We introduce userspace that can deal with before and
> after. After 1 or 2 years, we flip the switch. After another 1 or 2
> years, we get rid of backward compatibility.
> 
> 	Third possibility : we declare 32 bit userspace on 64 bit
> kernel as not supported and advise users to get a 64 bit
> userspace. The number of bug report on that issue would suggest that
> very few users are in this case.

I think that this is not actually an option since
powerpc64 is all 32-bit userspace.
Maybe some other arch-es are like this also (?).

> 	I know the userspace guys will hate (1) and hate even more (2).
> 
> 	Regards,
> 
> 	Jean



---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Thu, Mar 08, 2007 at 02:17:56PM -0800, Randy Dunlap wrote:
> On Thu, 8 Mar 2007 14:11:28 -0800 Jean Tourrilhes wrote:
> > 
> > 	Third possibility : we declare 32 bit userspace on 64 bit
> > kernel as not supported and advise users to get a 64 bit
> > userspace. The number of bug report on that issue would suggest that
> > very few users are in this case.
> 
> I think that this is not actually an option since
> powerpc64 is all 32-bit userspace.
> Maybe some other arch-es are like this also (?).

	Then, I assume that the powerpc64 must be using a magic
version of iwconfig to configure wireless interfaces, because I only
ever got one other bug report on the issue. They should have sent the
patch to Johannes, because now I know about their unauthorised version
of iwconfig.
	Thanks,

	Jean

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 14:30 -0800, Jean Tourrilhes wrote:

> 	Then, I assume that the powerpc64 must be using a magic
> version of iwconfig to configure wireless interfaces, because I only
> ever got one other bug report on the issue. They should have sent the
> patch to Johannes, because now I know about their unauthorised version
> of iwconfig.

There probably aren't many powerpc64 users that actually use wireless on
it since, well, most such machines are pretty stationary and
well-equipped with gigabit ethernet :)

May I ask who this was though? Some distro?

johannes

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

Re: wireless extensions vs. 64-bit architectures

[David Miller]

From: Randy Dunlap <randy.dunlap-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Date: Thu, 8 Mar 2007 14:17:56 -0800

> I think that this is not actually an option since
> powerpc64 is all 32-bit userspace.
> Maybe some other arch-es are like this also (?).

sparc64 is like this too

Re: wireless extensions vs. 64-bit architectures

[Pavel Roskin]

On Thu, 2007-03-08 at 14:17 -0800, Randy Dunlap wrote:
> I think that this is not actually an option since
> powerpc64 is all 32-bit userspace.
> Maybe some other arch-es are like this also (?).

I think all other architectures except x86_64 and maybe ia64 would
prefer to stay 32-bit for performance reasons alone.  As for x86_64 and
ia64, there is another incentive, namely compatibility with x86, which
matters if proprietary software is involved.  Finally, using 32-bit
userspace could cut memory consumption, which is important for some
uses.

Switching 32-bit systems to a 64-bit kernel shouldn't be a big deal.  It
should be transparent, just like enabling an option to support 4
gigabytes of memory or 64-bit PCI resources.

32-bit distributions should have an option to install a 64-bit kernel,
just like it's possible to install a kernel optimized for 586 CPU.  A
Live CD could benefit from 64-bit kernel because it would allow users to
chroot to their 64-bit distro installation and repair it, without having
to provide 64-bit userspace on the CD.

I think the reason 32-bit userspace on 64-bit kernel is not widespread
is precisely because of such incompatibilities as the one we are
discussing.  The need for proper support will grow as laptops with over
1 gigabyte of memory become a commonplace.

I believe breaking the "u32/k64" compatibility is not an option. I would
prefer the option two, the changeover.  I don't think wireless
extensions (or at least the compatible kernel API) should go away soon.

-- 
Regards,
Pavel Roskin

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 14:11 -0800, Jean Tourrilhes wrote:

> 	This is exactly what I was pointing out earlier. Well,
> actually, there may be ways of fixing it in the kernel, but that would
> be real ugly, and I don't want to go there.

Yeah, it would be extremely ugly and involve a lot of copying around
when the app actually receives the data.

> 	I've just released wireless_tools.29.pre15.tar.gz. This is
> supposed to include a "band-aid" for that problem. To the best of my
> knowledge, it should catch the problem and not introduce false
> positive. I would be glad if you guys would have a quick look into it,
> because obviously I can't test it.

I looked at the diff between pre14 and pre15 but have to admit that I
don't understand the code change. Unfortunately I can't test this until
the 20th earliest so it'd be good if someone else could test this.

Btw, could you look at the information leak patch I posted? We really
need to get that fix (or another appropriate one) out asap.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Thu, Mar 08, 2007 at 11:22:06PM +0100, Johannes Berg wrote:
> On Thu, 2007-03-08 at 14:11 -0800, Jean Tourrilhes wrote:
> 
> > 	This is exactly what I was pointing out earlier. Well,
> > actually, there may be ways of fixing it in the kernel, but that would
> > be real ugly, and I don't want to go there.
> 
> Yeah, it would be extremely ugly and involve a lot of copying around
> when the app actually receives the data.
> 
> > 	I've just released wireless_tools.29.pre15.tar.gz. This is
> > supposed to include a "band-aid" for that problem. To the best of my
> > knowledge, it should catch the problem and not introduce false
> > positive. I would be glad if you guys would have a quick look into it,
> > because obviously I can't test it.
> 
> I looked at the diff between pre14 and pre15 but have to admit that I
> don't understand the code change.

	I must admit the fix is ugly.

> Unfortunately I can't test this until
> the 20th earliest so it'd be good if someone else could test this.

	Ok.

> Btw, could you look at the information leak patch I posted? We really
> need to get that fix (or another appropriate one) out asap.

	IMHO this does not have the same urgency. I'll see if we could
avoid copying, because the dest buffer is already zero filled.

> johannes

	Have fun...

	Jean

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Thu, 2007-03-08 at 14:11 -0800, Jean Tourrilhes wrote:

> 	First possiblity, we could stick with this band-aid
> permanently.

It sucks for various reasons, one for example being that I don't even
understand your recognition code but all userspace programs that use
wext will have to include such code!

> 	Second possiblity : we do the right thing and plan a API
> change to return struct always aligned on 32 bits. This way, when we
> get 128 bit processor, we don't have to add another band aid ;-)
> 	It would work like the ESSID changeover. We pick a WE version
> changeover. We introduce userspace that can deal with before and
> after. After 1 or 2 years, we flip the switch. After another 1 or 2
> years, we get rid of backward compatibility.

Probably doesn't make much sense, we might as well schedule wext for
removal by then and just make the effort to replace at least for all
IEEE 802.11 hardware. And then we rip out the compat ioctls completely
(just deny them) so if someone's stupid enough to use really really old
hw in new machines (where that even works) they need a 64-bit userspace.

> 	Third possibility : we declare 32 bit userspace on 64 bit
> kernel as not supported and advise users to get a 64 bit
> userspace. The number of bug report on that issue would suggest that
> very few users are in this case.

Randy is right here, most a lot of powerpc64 users don't bother
compiling 64-bit userspace apps since there's not much point in it.
Also, a lot of distros don't even distinguish between powerpc32 and
powerpc64 so debian for example can't possibly fix this by shipping
64-bit versions of the affected tools. Besides, a 64-bit network manager
couldn't integrate with gnome-panel I'd think.

The only real fix to me is fixing it in the kernel, but as you say
that's very icky.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Thu, Mar 08, 2007 at 11:35:26PM +0100, Johannes Berg wrote:
> On Thu, 2007-03-08 at 14:11 -0800, Jean Tourrilhes wrote:
> 
> > 	First possiblity, we could stick with this band-aid
> > permanently.
> 
> It sucks for various reasons, one for example being that I don't even
> understand your recognition code but all userspace programs that use
> wext will have to include such code!

	It's not as bad as it look like. All userspace programs
nowadays use either the iwlib or wpa_supplicant. For example,
NetworkManager gets its stuff through wpa_supplicant, and kdenetwork
uses iwlib. So, in essence, there is only two places to fix.
	Which is why I would like to hear from Jouni.

	By the way, I've just released version 29.pre16 which has a
better band aid. It fixes bugs of the previous version and is more
forward compatible (i.e. it's compatible with option 2).

	Have fun...

	Jean

Re: wireless extensions vs. 64-bit architectures

[Jouni Malinen]

On Fri, Mar 09, 2007 at 01:35:31PM -0800, Jean Tourrilhes wrote:

> 	It's not as bad as it look like. All userspace programs
> nowadays use either the iwlib or wpa_supplicant. For example,
> NetworkManager gets its stuff through wpa_supplicant, and kdenetwork
> uses iwlib. So, in essence, there is only two places to fix.
> 	Which is why I would like to hear from Jouni.

How confident are you of this being the full set? I did not use iwlib
for two reasons: its license and it not being available (at least at
that point in time) by default in many Linux distros. I would expect
someone else having the same reasons, but I have to admit that I have
not looked into this at all, so this is only a guess.

> 	By the way, I've just released version 29.pre16 which has a
> better band aid. It fixes bugs of the previous version and is more
> forward compatible (i.e. it's compatible with option 2).

Hmm.. What should I look as the description of the latest proposed
workaround? I was kind of hoping to see a nice description of the
problem and proposed change in couple of paragraphs of text.. ;-)
Anyway, I can see two changes when I diff wireless tools 29.pre14 and
pre15 that are clearly related to this. Diff of pre14 vs. pre16 or pre15
vs. pre16 includes more changes and some of them do not seem to be
related to issue.

Unfortunately I do not have access to any system to test this
workaround, so I cannot easily test this myself.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Fri, Mar 09, 2007 at 03:19:22PM -0800, Jouni Malinen wrote:
> On Fri, Mar 09, 2007 at 01:35:31PM -0800, Jean Tourrilhes wrote:
> 
> > 	It's not as bad as it look like. All userspace programs
> > nowadays use either the iwlib or wpa_supplicant. For example,
> > NetworkManager gets its stuff through wpa_supplicant, and kdenetwork
> > uses iwlib. So, in essence, there is only two places to fix.
> > 	Which is why I would like to hear from Jouni.
> 
> How confident are you of this being the full set?

	I spent some time tracking that when we did the ESSID
change. I also try to keep the list on my web site up to date.
	The reason the set is small is that most programs are
simplistic, only reporting signal strength, and only a few go beyond
that.

> > 	By the way, I've just released version 29.pre16 which has a
> > better band aid. It fixes bugs of the previous version and is more
> > forward compatible (i.e. it's compatible with option 2).
> 
> Hmm.. What should I look as the description of the latest proposed
> workaround? I was kind of hoping to see a nice description of the
> problem and proposed change in couple of paragraphs of text.. ;-)

	Alignement issue in structures. On 64 bit, you get an extra 4
bytes of stuff between the header and the payload. So, you need to
detect this condition and remove the extra 4 bytes.

> Anyway, I can see two changes when I diff wireless tools 29.pre14 and
> pre15 that are clearly related to this. Diff of pre14 vs. pre16 or pre15
> vs. pre16 includes more changes and some of them do not seem to be
> related to issue.

	Actually, apart from ifrename, all the pre15 to pre16 changes
are related to this.

> Unfortunately I do not have access to any system to test this
> workaround, so I cannot easily test this myself.

	Same here.

> Jouni Malinen

	Jean

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Fri, 2007-03-09 at 13:35 -0800, Jean Tourrilhes wrote:

> 	It's not as bad as it look like. All userspace programs
> nowadays use either the iwlib or wpa_supplicant. For example,
> NetworkManager gets its stuff through wpa_supplicant, and kdenetwork
> uses iwlib. So, in essence, there is only two places to fix.

That's definitely not true, the latest network manager I have sources to
on my laptop right now (0.6.4) uses iwlib but only iw_get_ext and no
parsing functions. I would guess that others "use" iwlib like that too.

> 	By the way, I've just released version 29.pre16 which has a
> better band aid. It fixes bugs of the previous version and is more
> forward compatible (i.e. it's compatible with option 2).

Can you please also *document* the bandaid and actually *describe* it in
*words* instead of dense code without comments?

I'm still not convinced that papering over the problem in userspace is a
real solution.

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Ulrich Kunitz]

> I'm still not convinced that papering over the problem in userspace is a
> real solution.
> 
> johannes

Just my 2 cents. I support this. What are the options? I see only
two:

1. Use different magic numbers for 32 bit and 64 bit structures. A
   flag is an alternative, but will be more difficult to debug.
   Generation of the magic should be easy, use sizeof(unsigned
   long) as test. User space has to care than for the rest.

2. Make the data representation identical in 32 bit and 64 bit.

   This shouldn't be to difficult, if only u8, u16 and u32 types
   are used. Pointers should be given as offsets. If necessary
   align and/or packed attributes could be used.

If the kernel interface can be changed, I vote for option 2,
because user space has then to deal with a unique data layout.
If the wext kernel interface cannot be changed to maintain
backward compatibility, then I have to admit band-aids in user
space are needed. However cfg80211 must not suffer from the same issues.

-- 
Uli Kunitz

Re: wireless extensions vs. 64-bit architectures

[Michael Buesch]

On Sunday 11 March 2007 21:11, Ulrich Kunitz wrote:
> > I'm still not convinced that papering over the problem in userspace is a
> > real solution.
> > 
> > johannes
> 
> Just my 2 cents. I support this. What are the options? I see only
> two:
> 
> 1. Use different magic numbers for 32 bit and 64 bit structures. A
>    flag is an alternative, but will be more difficult to debug.
>    Generation of the magic should be easy, use sizeof(unsigned
>    long) as test. User space has to care than for the rest.
> 
> 2. Make the data representation identical in 32 bit and 64 bit.
> 
>    This shouldn't be to difficult, if only u8, u16 and u32 types
>    are used.

>    Pointers should be given as offsets.

Offsets to what?

>    If necessary  
>    align and/or packed attributes could be used.
> 
> If the kernel interface can be changed, I vote for option 2,
> because user space has then to deal with a unique data layout.
> If the wext kernel interface cannot be changed to maintain
> backward compatibility, then I have to admit band-aids in user
> space are needed. However cfg80211 must not suffer from the same issues.

All your suggestions break the userspace ABI on all machines,
which is a thing we _really_ want to avoid.

The right fix is to have a compatibility wrapper in the kernel,
but it turns out to be hard to implement. I don't think we want
to change the structures layout and break ABI.

-- 
Greetings Michael.

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Sun, Mar 11, 2007 at 06:40:01PM +0100, Johannes Berg wrote:
> On Fri, 2007-03-09 at 13:35 -0800, Jean Tourrilhes wrote:
> 
> > 	It's not as bad as it look like. All userspace programs
> > nowadays use either the iwlib or wpa_supplicant. For example,
> > NetworkManager gets its stuff through wpa_supplicant, and kdenetwork
> > uses iwlib. So, in essence, there is only two places to fix.
> 
> That's definitely not true, the latest network manager I have sources to
> on my laptop right now (0.6.4) uses iwlib but only iw_get_ext and no
> parsing functions.

	You are right. I did assume that if it was using iwlib, it
would use the scan helpers. As I said, I did my audit for the ESSID
stuff and did not refresh for the scan stuff.

> I would guess that others "use" iwlib like that too.

	Which others ? The applications that process scan results can
be counted on your fingers. And if you count the one actively
developped, you can use one hand.

> Can you please also *document* the bandaid and actually *describe* it in
> *words* instead of dense code without comments?

	I did that in the e-mail to Jouni. The problem is that most
people are unfamiliar with decoding iwevents, so can't grasp the
explanation.
	Basically, for iwpoint, we have an outer lenght and an inner
length. If they don't match, we have an alignement issue and just need
to pick the payload 8 bytes after the expected location.
	For other events, they have a well known size. If the outer
lenght is not the expected size, but is expected+4, you just pick the
payload 4 bytes after the expected location.

> I'm still not convinced that papering over the problem in userspace is a
> real solution.

	The final overall solution may be a mix of individual
solutions. This is definitely part of it. We don't know yet what are
the other part of this overall solution.
	This brings a solution to people using current and old
kernel. For example, I would expect to be much easier to convince
distro to update older/stable versions with a new wtool than with a
new kernel. The fix I did is trivial to backport to WT-28 or WT-27
(which I plan to do once it's confirmed it works).

>  johannes

	Have fun...

	Jean

Re: wireless extensions vs. 64-bit architectures

[Jouni Malinen]

On Mon, Mar 12, 2007 at 10:56:39AM -0700, Jean Tourrilhes wrote:

> > I would guess that others "use" iwlib like that too.
> 
> 	Which others ? The applications that process scan results can
> be counted on your fingers. And if you count the one actively
> developped, you can use one hand.

Quick search of SIOCGIWSCAN with Google Code Search
(http://www.google.com/codesearch) shows quite large set of uses. Most
of them are from various drivers, but at least some (e.g., Xsupplicant,
waproamd) are from user space applications. I did not actually verify
whether any of these would be affected by the 64/32-bit issue, but I
would assume some of the apps are indeed parsing the scan results with
internal implementation.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Mon, Mar 12, 2007 at 10:21:49AM -0800, Jouni Malinen wrote:
> On Mon, Mar 12, 2007 at 10:56:39AM -0700, Jean Tourrilhes wrote:
> 
> > > I would guess that others "use" iwlib like that too.
> > 
> > 	Which others ? The applications that process scan results can
> > be counted on your fingers. And if you count the one actively
> > developped, you can use one hand.
> 
> Quick search of SIOCGIWSCAN with Google Code Search
> (http://www.google.com/codesearch) shows quite large set of uses. Most
> of them are from various drivers, but at least some (e.g., Xsupplicant,
> waproamd) are from user space applications. I did not actually verify
> whether any of these would be affected by the 64/32-bit issue, but I
> would assume some of the apps are indeed parsing the scan results with
> internal implementation.

	Note that you will get such a list on my web page, and if you
find something not on the list, please send it to me.
	XSupplicant uses iwlib for scanning.
	waproamd has not been actively developped for the last 3 years.
	kdenetwork/KWiFiManager uses iwlib for scanning.
	Kifi uses iwlib and has not been actively developped for the
last 2 years.
	ApRadar has not been actively developped for the last 3 years.
	SuSE netapplet is no longer on the net. wifi-radar parse the
output of iwlist.
	Mandrake net_applet, from what I can gather, use wpa_supplicant.

	Ok, we are left with :
	wpa_supplicant use its own code.
	NetworkManager : partially use iwlib, use own code for scanning.

> Jouni Malinen

	Jean

Re: wireless extensions vs. 64-bit architectures

[Johannes Berg]

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

On Mon, 2007-03-12 at 10:56 -0700, Jean Tourrilhes wrote:

> 	I did that in the e-mail to Jouni. The problem is that most
> people are unfamiliar with decoding iwevents, so can't grasp the
> explanation.
> 	Basically, for iwpoint, we have an outer lenght and an inner
> length. If they don't match, we have an alignement issue and just need
> to pick the payload 8 bytes after the expected location.
> 	For other events, they have a well known size. If the outer
> lenght is not the expected size, but is expected+4, you just pick the
> payload 4 bytes after the expected location.

Ok.

So the plan now is to put this document up somewhere maybe with some
graphics or whatever, and then send this to distros so they know what
happens when people hit this bug.

Does your new version work without padding even on 64-bit arches? Then
in a few years we can actually remove the padding completely in the
kernel, right?

johannes

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

Re: wireless extensions vs. 64-bit architectures

[Jean Tourrilhes]

On Tue, Mar 13, 2007 at 08:42:05PM +0100, Johannes Berg wrote:
> On Mon, 2007-03-12 at 10:56 -0700, Jean Tourrilhes wrote:
> 
> > 	I did that in the e-mail to Jouni. The problem is that most
> > people are unfamiliar with decoding iwevents, so can't grasp the
> > explanation.
> > 	Basically, for iwpoint, we have an outer lenght and an inner
> > length. If they don't match, we have an alignement issue and just need
> > to pick the payload 8 bytes after the expected location.
> > 	For other events, they have a well known size. If the outer
> > lenght is not the expected size, but is expected+4, you just pick the
> > payload 4 bytes after the expected location.
> 
> Ok.
> 
> So the plan now is to put this document up somewhere maybe with some
> graphics or whatever, and then send this to distros so they know what
> happens when people hit this bug.
> 
> Does your new version work without padding even on 64-bit arches? Then
> in a few years we can actually remove the padding completely in the
> kernel, right?

	You are too smart ;-) Yes, the second version in pre16 does
exactly that. That's why I had to change the constants.

> johannes

	Jean