Re: Request to postpone WE-21

[Jean Tourrilhes]

On Tue, Oct 10, 2006 at 03:40:04PM -0400, John W. Linville wrote:
> 
> I think this patch still has two problems.  One is that the length
> modification does not happen until after the "Check what user space
> is giving us" clause.  So, max length requests will fail.  (Did you
> check SIOCGIWESSID w/ WE-20 tools?  Or am I missing something?
> SIOCGIWESSID and SIOCGIWNICN do not have IW_DESCR_FLAG_NOMAX set.)

Yes, I can see that SET with 32 char would get rejected. I did
not check 32 char with WE-20 tools. I don't know how widely 32 char
ESSID are used in the real world.

Over the week end, I tought about another potential gotcha,
however I don't think it would happen in practice. A tool could send a
non NUL terminated ESSID with WE-20, just adding a dummy char at the
end of the ESSID. That would work with most drivers.

> Since I think we have to account for the gets as well as the sets,
> then we have to restore the length value in the ifreq to be compatible
> with userland, just in case they reuse the data structure.

The GET API change *already* happened last January. It's
already a reality, and WE-21 did not really change anything about
that. This means that it's already included in "2.6.16", which is the
"somewhat more stable" kernel, and widely diffused.
There was userspace issues with that. Those have been fixed a
long time ago. I don't see the point of carrying code for those cases.

> I think the main issues with my patch were an off-by-one in the length
> for checking for \0, and a failure to account for a length of zero.
> I have attempted to correct those in my patch below.  What do you
> think?
> 
> John
> 
> P.S.  Again, this is an untested patch...

Compile Wireless Tools 27 with a static iwlib (top of
Makefile) to do your testing, so that you can run without having to
install it. And use a driver that care about the ending NUL, such as
the Broadcom or IPW2x00.

Have fun...

Jean
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[John W. Linville]

On Thu, Oct 05, 2006 at 03:12:46PM -0700, Jean Tourrilhes wrote:
> On Thu, Oct 05, 2006 at 04:49:54PM -0400, John W. Linville wrote:
> > On Thu, Oct 05, 2006 at 09:31:13AM -0700, Jean Tourrilhes wrote:
> > 
> > >   Based on the feedback, I formally request you to back out all
> > > of WE-21 from 2.6.19. Rationale : it's probably too early. You can
> > > keep it for a later date if you wish.
> > 
> > Jean,
> > 
> > What about a patch like the one below?  It tries to detect WE-20
> > ESSID/NICKN accesses and adjust them to WE-21 style.  What am
> > I missing?
> > 
> > I haven't had a chance to test it yet -- just hacked it
> > up...YMMV... :-)
> > 
> > John
> 
>   This is a tested and simplified version of your patch. I added
> an explicit "warning" message, so that users have visibility on what's
> happening.
>   I had a bit of fun on the testing phase, as most of my antique
> cards were just discarding the extra '\0'. The Broadcom did show the
> issue, and that this patch was fixing it properly.
>   I let you decide the best course of action.

I think this patch still has two problems.  One is that the length
modification does not happen until after the "Check what user space
is giving us" clause.  So, max length requests will fail.  (Did you
check SIOCGIWESSID w/ WE-20 tools?  Or am I missing something?
SIOCGIWESSID and SIOCGIWNICN do not have IW_DESCR_FLAG_NOMAX set.)

Since I think we have to account for the gets as well as the sets,
then we have to restore the length value in the ifreq to be compatible
with userland, just in case they reuse the data structure.

I think the main issues with my patch were an off-by-one in the length
for checking for \0, and a failure to account for a length of zero.
I have attempted to correct those in my patch below.  What do you
think?

John

P.S.  Again, this is an untested patch...

diff --git a/net/core/wireless.c b/net/core/wireless.c
index 0da..cb1b872 100644
--- a/net/core/wireless.c
+++ b/net/core/wireless.c
@@ -748,11 +748,39 @@ #endif/* WE_SET_EVENT */
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 */
@@ -795,7 +823,8 @@ #ifdef WE_IOCTL_DEBUG
 #endif /* WE_IOCTL_DEBUG */
 
/* Create the kernel buffer */
-   extra = kmalloc(extra_size, GFP_KERNEL);
+   /*kzalloc ensures NULL-termination for essid_compat */
+   extra = kzalloc(extra_size, GFP_KERNEL);
if (extra == NULL) {
return -ENOMEM;
}
@@ -819,6 +848,8 @@ #endif  /* WE_IOCTL_DEBUG */
/* 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 */
-- 
John W. Linville
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[Jeff Garzik]

Jouni Malinen wrote:

This looks somewhat confusing.. WE-20 (and older) included '\0' in both
the data value and length (well, at least in most drivers and user space
tools, if I remember correctly), i.e., essid[iwr->u.data.length] would
be pointing one byte after the '\0' termination.. And since '\0' is
valid character in SSID (it is just an arbitrary array of octets) it can
also be the last octet of the SSID and WE-21 style case could have
essid[iwr->u.data.length - 1] == '\0'..



Remember, the salient point is ensuring that WE<=20 continues to work as 
expecting, without any modification.  If that means a compromise in 
supported SSID values, so be it.  Just like older versions of stat(2) 
syscall, you are stuck with the old interface, warts included.


But if we can support both styles... great!  I'm all for it.  Just 
noting priorities.  Warts and limitations are inevitable with older 
interfaces.


Jeff


-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[Jean Tourrilhes]

On Thu, Oct 05, 2006 at 03:15:50PM -0700, Jouni Malinen wrote:
> On Thu, Oct 05, 2006 at 03:12:46PM -0700, Jean Tourrilhes wrote:
> > +   if((cmd == SIOCSIWESSID) ||
> > +  (cmd == SIOCSIWNICKN)) {
> > +   if(extra[iwr->u.data.length - 1] == '\0') {
> 
> Can't iwr->u.data.length be zero here (with WE-21)? Maybe better add
> 'iwr->u.data.length > 0 &&'..

It's already implied. I did not put the code in the same place
a John.
It looks like :
--
/* If it is a SET, get all the extra data in here */
if(IW_IS_SET(cmd) && (iwr->u.data.length != 0)) {
err = copy_from_user(extra, iwr->u.data.pointer,
 iwr->u.data.length *
 descr->token_size);

if((cmd == SIOCSIWESSID) ||
   (cmd == SIOCSIWNICKN)) {
if(extra[iwr->u.data.length - 1] == '\0') {
iwr->u.data.length--;
}
}
}
--

> Jouni Malinen

Thanks for the quick review !

Jean

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[Jean Tourrilhes]

On Thu, Oct 05, 2006 at 04:49:54PM -0400, John W. Linville wrote:
> On Thu, Oct 05, 2006 at 09:31:13AM -0700, Jean Tourrilhes wrote:
> 
> > Based on the feedback, I formally request you to back out all
> > of WE-21 from 2.6.19. Rationale : it's probably too early. You can
> > keep it for a later date if you wish.
> 
> Jean,
> 
> What about a patch like the one below?  It tries to detect WE-20
> ESSID/NICKN accesses and adjust them to WE-21 style.  What am
> I missing?
> 
> I haven't had a chance to test it yet -- just hacked it
> up...YMMV... :-)
> 
> John

This is a tested and simplified version of your patch. I added
an explicit "warning" message, so that users have visibility on what's
happening.
I had a bit of fun on the testing phase, as most of my antique
cards were just discarding the extra '\0'. The Broadcom did show the
issue, and that this patch was fixing it properly.
I let you decide the best course of action.

Have fun...

Jean

Signed-off-by: Jean Tourrilhes <[EMAIL PROTECTED]>

--

diff -u -p linux/net/core/wireless.j1.c linux/net/core/wireless.c
--- linux/net/core/wireless.j1.c2006-10-05 14:24:16.0 -0700
+++ linux/net/core/wireless.c   2006-10-05 14:53:23.0 -0700
@@ -821,6 +821,26 @@ static int ioctl_standard_call(struct ne
   dev->name,
   iwr->u.data.length * descr->token_size);
 #endif /* WE_IOCTL_DEBUG */
+
+   /* Some users have old userspace compatible
+* with WE-20. Those add an extra '\0' at the
+* end of the ESSID. Just get rids of it.
+* Note : in the long term we will need to get
+* rid of this code to allow '\0' as a valid
+* last char of the ESSID.
+* Jean II */
+   if((cmd == SIOCSIWESSID) ||
+  (cmd == SIOCSIWNICKN)) {
+   if(extra[iwr->u.data.length - 1] == '\0') {
+   static int printed_message;
+
+   if (!printed_message++)
+   printk(KERN_DEBUG "%s (WE) : 
NUL terminated ESSID detected, please update Wireless Tools !\n",
+  dev->name);
+
+   iwr->u.data.length--;
+   }
+   }
}
 
/* Call the handler */
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[Jouni Malinen]

On Thu, Oct 05, 2006 at 03:12:46PM -0700, Jean Tourrilhes wrote:
> + if((cmd == SIOCSIWESSID) ||
> +(cmd == SIOCSIWNICKN)) {
> + if(extra[iwr->u.data.length - 1] == '\0') {

Can't iwr->u.data.length be zero here (with WE-21)? Maybe better add
'iwr->u.data.length > 0 &&'..

-- 
Jouni MalinenPGP id EFC895FA
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[Jouni Malinen]

On Thu, Oct 05, 2006 at 04:49:54PM -0400, John W. Linville wrote:

> What about a patch like the one below?  It tries to detect WE-20
> ESSID/NICKN accesses and adjust them to WE-21 style.  What am
> I missing?

> diff --git a/net/core/wireless.c b/net/core/wireless.c

> + else if (IW_IS_SET(cmd)) {
> + char essid[IW_ESSID_MAX_SIZE + 1];
> +
> + err = copy_from_user(essid, iwr->u.data.pointer,
> +  iwr->u.data.length *
> +  descr->token_size);

> + if (essid[iwr->u.data.length] == '\0')
> + essid_compat = 1;

This looks somewhat confusing.. WE-20 (and older) included '\0' in both
the data value and length (well, at least in most drivers and user space
tools, if I remember correctly), i.e., essid[iwr->u.data.length] would
be pointing one byte after the '\0' termination.. And since '\0' is
valid character in SSID (it is just an arbitrary array of octets) it can
also be the last octet of the SSID and WE-21 style case could have
essid[iwr->u.data.length - 1] == '\0'..

-- 
Jouni MalinenPGP id EFC895FA
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[Jean Tourrilhes]

On Thu, Oct 05, 2006 at 02:57:51PM -0700, Jouni Malinen wrote:
> On Thu, Oct 05, 2006 at 04:49:54PM -0400, John W. Linville wrote:
> 
> > What about a patch like the one below?  It tries to detect WE-20
> > ESSID/NICKN accesses and adjust them to WE-21 style.  What am
> > I missing?
> 
> > diff --git a/net/core/wireless.c b/net/core/wireless.c
> 
> > +   else if (IW_IS_SET(cmd)) {
> > +   char essid[IW_ESSID_MAX_SIZE + 1];
> > +
> > +   err = copy_from_user(essid, iwr->u.data.pointer,
> > +iwr->u.data.length *
> > +descr->token_size);
> 
> > +   if (essid[iwr->u.data.length] == '\0')
> > +   essid_compat = 1;
> 
> This looks somewhat confusing.. WE-20 (and older) included '\0' in both
> the data value and length (well, at least in most drivers and user space
> tools, if I remember correctly), i.e., essid[iwr->u.data.length] would
> be pointing one byte after the '\0' termination..

Obviously. John's code was only a proof of concept, tested
patch is coming in another e-mail.

> And since '\0' is
> valid character in SSID (it is just an arbitrary array of octets) it can
> also be the last octet of the SSID and WE-21 style case could have
> essid[iwr->u.data.length - 1] == '\0'..

I think we will have to suffer a bit. After the big mess
created by the whole story, I think it might not be a bad idea to get
90% of the way there, and add the remaining 10% of a later date. At
the rate userspace is progressing, it's only a matter of weeks.

> Jouni Malinen

Have fun...

Jean

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[Jean Tourrilhes]

On Thu, Oct 05, 2006 at 04:49:54PM -0400, John W. Linville wrote:
> On Thu, Oct 05, 2006 at 09:31:13AM -0700, Jean Tourrilhes wrote:
> 
> > Based on the feedback, I formally request you to back out all
> > of WE-21 from 2.6.19. Rationale : it's probably too early. You can
> > keep it for a later date if you wish.
> 
> Jean,

Let me say I truly apreciate your effort to bring progress to
the discussion.

> What about a patch like the one below?  It tries to detect WE-20
> ESSID/NICKN accesses and adjust them to WE-21 style.  What am
> I missing?

The idea is clever.
The GET is no longer an issue. WE had half the driver doing
the GET "new style" since january, so in a sense the API change has
already happened, and I've already dealt with the bug reports. So, I
think we could drop the "GET" part.
As you may have noticed, detecting the API for the GET is
easy. On the other hand, detecting it for the SET is not clear cut. As
Jouni was pointing out, '\0' is a valid ESSID character, and in the
long term we want to allow it, even if it's in the last position.
I'm also wondering if this additional complexity could not
bring additional trouble, but I'm not currently clear on that. I
usually prefer things to be a bit more explicit.

> I haven't had a chance to test it yet -- just hacked it
> up...YMMV... :-)

And I thing there is a couple of way we could refine the
implementation, if ever we decide to go that way.
For example, the correction could happen after real
copy_from_user(), as the uncorrected iwr->u.data.length is always the
number of char to pass between kernel and userspace. I think this
would simplify drastically the code.
I'll try to check that.

> John

Thanks again...

Jean
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Request to postpone WE-21

[John W. Linville]

On Thu, Oct 05, 2006 at 09:31:13AM -0700, Jean Tourrilhes wrote:

>   Based on the feedback, I formally request you to back out all
> of WE-21 from 2.6.19. Rationale : it's probably too early. You can
> keep it for a later date if you wish.

Jean,

What about a patch like the one below?  It tries to detect WE-20
ESSID/NICKN accesses and adjust them to WE-21 style.  What am
I missing?

I haven't had a chance to test it yet -- just hacked it
up...YMMV... :-)

John

diff --git a/net/core/wireless.c b/net/core/wireless.c
index 0da..ba5fe77 100644
--- a/net/core/wireless.c
+++ b/net/core/wireless.c
@@ -748,11 +748,39 @@ #endif/* WE_SET_EVENT */
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)) {
+   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] == '\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 */
@@ -795,7 +823,8 @@ #ifdef WE_IOCTL_DEBUG
 #endif /* WE_IOCTL_DEBUG */
 
/* Create the kernel buffer */
-   extra = kmalloc(extra_size, GFP_KERNEL);
+   /*kzalloc ensures NULL-termination for essid_compat */
+   extra = kzalloc(extra_size, GFP_KERNEL);
if (extra == NULL) {
return -ENOMEM;
}
@@ -819,6 +848,8 @@ #endif  /* WE_IOCTL_DEBUG */
/* 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 */

-- 
John W. Linville
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Request to postpone WE-21

[Jean Tourrilhes]

Hi John,

Based on the feedback, I formally request you to back out all
of WE-21 from 2.6.19. Rationale : it's probably too early. You can
keep it for a later date if you wish.
Regards,

Jean
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html