2007-07-09 22:14:06

by Jean Tourrilhes

[permalink] [raw]
Subject: [PATCH 2.6] WE : Fix get 32 char ESSID

Hi,

A little bug was introduced a few months ago that prevent
reading ESSID with 32 character. Philippe Teuwen was the first one to
report that, followed by the MadWifi team :
http://madwifi.org/ticket/930
The patch fix this bug by removing obsolete code. It also
reenable setting the full range of ESSID, including ESSID with a final
NUL which are valid. This was tested on 2.6.21 and 2.6.22.
John : would you mind pushing that to 2.6.23 ? If you are
happy with it, I can provide backport patches for earlier kernels.
Thanks...

Jean

Signed-off-by: Jean Tourrilhes <[email protected]>

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

--- linux/net/wireless/wext.j1.c 2007-07-09 13:19:22.000000000 -0700
+++ linux/net/wireless/wext.c 2007-07-09 13:19:59.000000000 -0700
@@ -741,39 +741,11 @@ static int ioctl_standard_call(struct ne
int extra_size;
int user_length = 0;
int err;
- int essid_compat = 0;

/* Calculate space needed by arguments. Always allocate
* for max space. Easier, and won't last long... */
extra_size = descr->max_tokens * descr->token_size;

- /* Check need for ESSID compatibility for WE < 21 */
- switch (cmd) {
- case SIOCSIWESSID:
- case SIOCGIWESSID:
- case SIOCSIWNICKN:
- case SIOCGIWNICKN:
- if (iwr->u.data.length == descr->max_tokens + 1)
- essid_compat = 1;
- else if (IW_IS_SET(cmd) && (iwr->u.data.length != 0)) {
- char essid[IW_ESSID_MAX_SIZE + 1];
-
- err = copy_from_user(essid, iwr->u.data.pointer,
- iwr->u.data.length *
- descr->token_size);
- if (err)
- return -EFAULT;
-
- if (essid[iwr->u.data.length - 1] == '\0')
- essid_compat = 1;
- }
- break;
- default:
- break;
- }
-
- iwr->u.data.length -= essid_compat;
-
/* Check what user space is giving us */
if (IW_IS_SET(cmd)) {
/* Check NULL pointer */
@@ -811,7 +783,6 @@ static int ioctl_standard_call(struct ne
}

/* Create the kernel buffer */
- /* kzalloc ensures NULL-termination for essid_compat */
extra = kzalloc(extra_size, GFP_KERNEL);
if (extra == NULL)
return -ENOMEM;
@@ -830,8 +801,6 @@ static int ioctl_standard_call(struct ne
/* Call the handler */
ret = handler(dev, &info, &(iwr->u), extra);

- iwr->u.data.length += essid_compat;
-
/* If we have something to return to the user */
if (!ret && IW_IS_GET(cmd)) {
/* Check if there is enough buffer up there */



2007-07-10 17:01:22

by Jean Tourrilhes

[permalink] [raw]
Subject: Re: [PATCH 2.6] WE : Fix get 32 char ESSID

On Tue, Jul 10, 2007 at 11:10:44AM -0400, John W. Linville wrote:
> On Mon, Jul 09, 2007 at 03:13:22PM -0700, Jean Tourrilhes wrote:
>
> > A little bug was introduced a few months ago that prevent
> > reading ESSID with 32 character. Philippe Teuwen was the first one to
> > report that, followed by the MadWifi team :
> > http://madwifi.org/ticket/930
> > The patch fix this bug by removing obsolete code. It also
> > reenable setting the full range of ESSID, including ESSID with a final
> > NUL which are valid. This was tested on 2.6.21 and 2.6.22.
> > John : would you mind pushing that to 2.6.23 ? If you are
> > happy with it, I can provide backport patches for earlier kernels.
>
> Jean,
>
> Thanks for the patch.
>
> Iirc, I wrote this ugly little piece of code that you seek to remove.

Yes, I remember perfectly the context of that piece of code,
and I was assuming you did ;-) It was last autumn, and we said it was
temporary. As we have now found that it's buggy, it's a good time to
remove it.

> It's purpose is/was to preserve userland ABI for older versions of
> the wireless-tools package. I'll have to dig a little deeper if you
> need a more detailed refresher -- let me know.

If you want, I can fully refresh you ;-)

> In that case, I believe the issue was that using older wireless-tools
> w/ newer kernels would result in the SSID getting truncated by one
> character. If we apply this patch (removing my hack/fix), won't we
> be reintroducing that issue? If not, can you remind me as to why not?

Yes, we would definitely reintroduce the issue with older
versions of Wireless Tools. But, as this API has been deprecated for
now more than one year (february 2006), and as this patch causes
troubles, it is time to get rid of it.
These are the released distributions that have a "good"
version of Wireless Tools and wpa_supplicant :
Debian 4.0 Etch
Unbuntu 6.10 Edgy and 7.04 Feisty
Fedora 6.93 and 7.0
OpenSuSE 10.2
Mandriva 2007.0 and 2007.1
Slackware 11.0 and 12.0
As you can see, most distro have now 2 releases which are
"good"...

> Thanks,
>
> John

Regards,

Jean

2007-07-10 15:38:35

by John W. Linville

[permalink] [raw]
Subject: Re: [PATCH 2.6] WE : Fix get 32 char ESSID

On Mon, Jul 09, 2007 at 03:13:22PM -0700, Jean Tourrilhes wrote:

> A little bug was introduced a few months ago that prevent
> reading ESSID with 32 character. Philippe Teuwen was the first one to
> report that, followed by the MadWifi team :
> http://madwifi.org/ticket/930
> The patch fix this bug by removing obsolete code. It also
> reenable setting the full range of ESSID, including ESSID with a final
> NUL which are valid. This was tested on 2.6.21 and 2.6.22.
> John : would you mind pushing that to 2.6.23 ? If you are
> happy with it, I can provide backport patches for earlier kernels.

Jean,

Thanks for the patch.

Iirc, I wrote this ugly little piece of code that you seek to remove.
It's purpose is/was to preserve userland ABI for older versions of
the wireless-tools package. I'll have to dig a little deeper if you
need a more detailed refresher -- let me know.

In that case, I believe the issue was that using older wireless-tools
w/ newer kernels would result in the SSID getting truncated by one
character. If we apply this patch (removing my hack/fix), won't we
be reintroducing that issue? If not, can you remind me as to why not?

Thanks,

John

>
> Signed-off-by: Jean Tourrilhes <[email protected]>
>
> -----------------------------------------------------------
>
> --- linux/net/wireless/wext.j1.c 2007-07-09 13:19:22.000000000 -0700
> +++ linux/net/wireless/wext.c 2007-07-09 13:19:59.000000000 -0700
> @@ -741,39 +741,11 @@ static int ioctl_standard_call(struct ne
> int extra_size;
> int user_length = 0;
> int err;
> - int essid_compat = 0;
>
> /* Calculate space needed by arguments. Always allocate
> * for max space. Easier, and won't last long... */
> extra_size = descr->max_tokens * descr->token_size;
>
> - /* Check need for ESSID compatibility for WE < 21 */
> - switch (cmd) {
> - case SIOCSIWESSID:
> - case SIOCGIWESSID:
> - case SIOCSIWNICKN:
> - case SIOCGIWNICKN:
> - if (iwr->u.data.length == descr->max_tokens + 1)
> - essid_compat = 1;
> - else if (IW_IS_SET(cmd) && (iwr->u.data.length != 0)) {
> - char essid[IW_ESSID_MAX_SIZE + 1];
> -
> - err = copy_from_user(essid, iwr->u.data.pointer,
> - iwr->u.data.length *
> - descr->token_size);
> - if (err)
> - return -EFAULT;
> -
> - if (essid[iwr->u.data.length - 1] == '\0')
> - essid_compat = 1;
> - }
> - break;
> - default:
> - break;
> - }
> -
> - iwr->u.data.length -= essid_compat;
> -
> /* Check what user space is giving us */
> if (IW_IS_SET(cmd)) {
> /* Check NULL pointer */
> @@ -811,7 +783,6 @@ static int ioctl_standard_call(struct ne
> }
>
> /* Create the kernel buffer */
> - /* kzalloc ensures NULL-termination for essid_compat */
> extra = kzalloc(extra_size, GFP_KERNEL);
> if (extra == NULL)
> return -ENOMEM;
> @@ -830,8 +801,6 @@ static int ioctl_standard_call(struct ne
> /* Call the handler */
> ret = handler(dev, &info, &(iwr->u), extra);
>
> - iwr->u.data.length += essid_compat;
> -
> /* If we have something to return to the user */
> if (!ret && IW_IS_GET(cmd)) {
> /* Check if there is enough buffer up there */
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
John W. Linville
[email protected]