[PATCH] cfg80211: fix a couple of bugs with key ioctls

[PATCH] cfg80211: fix a couple of bugs with key ioctls

[Johannes Berg]

There are a few small bugs/oversights in the key handling
code I wrote:
 * SIOCSIWENCODE should default to setting key 0 if no
   default key is set already and no index is given,
 * key removal should not require key material,
 * SIOCSIWENCODEEXT should default to changing the default
   management key if no index is given.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/wireless/wext-compat.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

--- wireless-testing.orig/net/wireless/wext-compat.c	2009-05-12 12:38:41.000000000 +0200
+++ wireless-testing/net/wireless/wext-compat.c	2009-05-12 12:38:59.000000000 +0200
@@ -557,7 +557,7 @@ int cfg80211_wext_siwencode(struct net_d
 	if (idx == 0) {
 		idx = wdev->wext.default_key;
 		if (idx < 0)
-			return -EINVAL;
+			idx = 0;
 	} else if (idx < 1 || idx > 4)
 		return -EINVAL;
 	else
@@ -580,7 +580,7 @@ int cfg80211_wext_siwencode(struct net_d
 		params.cipher = WLAN_CIPHER_SUITE_WEP40;
 	else if (erq->length == 13)
 		params.cipher = WLAN_CIPHER_SUITE_WEP104;
-	else
+	else if (!remove)
 		return -EINVAL;
 
 	return cfg80211_set_encryption(rdev, dev, NULL, remove,
@@ -640,13 +640,9 @@ int cfg80211_wext_siwencodeext(struct ne
 	idx = erq->flags & IW_ENCODE_INDEX;
 	if (cipher == WLAN_CIPHER_SUITE_AES_CMAC) {
 		if (idx < 4 || idx > 5) {
-			/*
-			 * XXX: Only wpa_supplicant ever used this
-			 *	can we still change the ABI a little
-			 *	so we do not need to keep track of
-			 *	the default key?
-			 */
-			return -EINVAL;
+			idx = wdev->wext.default_mgmt_key;
+			if (idx < 0)
+				return -EINVAL;
 		} else
 			idx--;
 	} else {

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Hin-Tak Leung]

On Tue, May 12, 2009 at 11:44 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> There are a few small bugs/oversights in the key handling
> code I wrote:
> =A0* SIOCSIWENCODE should default to setting key 0 if no
> =A0 default key is set already and no index is given,
> =A0* key removal should not require key material,
> =A0* SIOCSIWENCODEEXT should default to changing the default
> =A0 management key if no index is given.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> =A0net/wireless/wext-compat.c | =A0 14 +++++---------
> =A01 file changed, 5 insertions(+), 9 deletions(-)
>
> --- wireless-testing.orig/net/wireless/wext-compat.c =A0 =A02009-05-1=
2 12:38:41.000000000 +0200
> +++ wireless-testing/net/wireless/wext-compat.c 2009-05-12 12:38:59.0=
00000000 +0200
> @@ -557,7 +557,7 @@ int cfg80211_wext_siwencode(struct net_d
> =A0 =A0 =A0 =A0if (idx =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0idx =3D wdev->wext.default_key;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (idx < 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 idx =3D 0;
> =A0 =A0 =A0 =A0} else if (idx < 1 || idx > 4)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> =A0 =A0 =A0 =A0else
> @@ -580,7 +580,7 @@ int cfg80211_wext_siwencode(struct net_d
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0params.cipher =3D WLAN_CIPHER_SUITE_WE=
P40;
> =A0 =A0 =A0 =A0else if (erq->length =3D=3D 13)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0params.cipher =3D WLAN_CIPHER_SUITE_WE=
P104;
> - =A0 =A0 =A0 else
> + =A0 =A0 =A0 else if (!remove)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
>
> =A0 =A0 =A0 =A0return cfg80211_set_encryption(rdev, dev, NULL, remove=
,
> @@ -640,13 +640,9 @@ int cfg80211_wext_siwencodeext(struct ne
> =A0 =A0 =A0 =A0idx =3D erq->flags & IW_ENCODE_INDEX;
> =A0 =A0 =A0 =A0if (cipher =3D=3D WLAN_CIPHER_SUITE_AES_CMAC) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (idx < 4 || idx > 5) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* XXX: Only wpa_supp=
licant ever used this
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0can we =
still change the ABI a little
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0so we d=
o not need to keep track of
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0the def=
ault key?
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 idx =3D wdev->wext.defa=
ult_mgmt_key;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (idx < 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return =
-EINVAL;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0idx--;
> =A0 =A0 =A0 =A0} else {
>
>
>

I tried this, NM/wpa_suplicany still doesn't work, but if I shut those
down and do it the old fashioned iwconfig way, I can get
connectivity...  the last chunk looks a bit funny - '(idx < 4 || idx >
5)' is just '(idx !=3D 4)' so why it isn't written simplier as such?
also the idx < 0 maybe <1 ?

Stiill needs some work...
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Hin-Tak Leung]

On Tue, May 12, 2009 at 8:05 PM, Hin-Tak Leung <hintak.leung@gmail.com>=
 wrote:
> On Tue, May 12, 2009 at 11:44 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> There are a few small bugs/oversights in the key handling
>> code I wrote:
>> =A0* SIOCSIWENCODE should default to setting key 0 if no
>> =A0 default key is set already and no index is given,
>> =A0* key removal should not require key material,
>> =A0* SIOCSIWENCODEEXT should default to changing the default
>> =A0 management key if no index is given.
>>
>> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>> ---
>> =A0net/wireless/wext-compat.c | =A0 14 +++++---------
>> =A01 file changed, 5 insertions(+), 9 deletions(-)
>>
>> --- wireless-testing.orig/net/wireless/wext-compat.c =A0 =A02009-05-=
12 12:38:41.000000000 +0200
>> +++ wireless-testing/net/wireless/wext-compat.c 2009-05-12 12:38:59.=
000000000 +0200
>> @@ -557,7 +557,7 @@ int cfg80211_wext_siwencode(struct net_d
>> =A0 =A0 =A0 =A0if (idx =3D=3D 0) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0idx =3D wdev->wext.default_key;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (idx < 0)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 idx =3D 0;
>> =A0 =A0 =A0 =A0} else if (idx < 1 || idx > 4)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
>> =A0 =A0 =A0 =A0else
>> @@ -580,7 +580,7 @@ int cfg80211_wext_siwencode(struct net_d
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0params.cipher =3D WLAN_CIPHER_SUITE_W=
EP40;
>> =A0 =A0 =A0 =A0else if (erq->length =3D=3D 13)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0params.cipher =3D WLAN_CIPHER_SUITE_W=
EP104;
>> - =A0 =A0 =A0 else
>> + =A0 =A0 =A0 else if (!remove)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
>>
>> =A0 =A0 =A0 =A0return cfg80211_set_encryption(rdev, dev, NULL, remov=
e,
>> @@ -640,13 +640,9 @@ int cfg80211_wext_siwencodeext(struct ne
>> =A0 =A0 =A0 =A0idx =3D erq->flags & IW_ENCODE_INDEX;
>> =A0 =A0 =A0 =A0if (cipher =3D=3D WLAN_CIPHER_SUITE_AES_CMAC) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (idx < 4 || idx > 5) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* XXX: Only wpa_sup=
plicant ever used this
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0can we=
 still change the ABI a little
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0so we =
do not need to keep track of
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0the de=
fault key?
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 idx =3D wdev->wext.def=
ault_mgmt_key;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (idx < 0)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return=
 -EINVAL;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0idx--;
>> =A0 =A0 =A0 =A0} else {
>>
>>
>>
>
> I tried this, NM/wpa_suplicany still doesn't work, but if I shut thos=
e
> down and do it the old fashioned iwconfig way, I can get
> connectivity... =A0the last chunk looks a bit funny - '(idx < 4 || id=
x >
> 5)' is just '(idx !=3D 4)' so why it isn't written simplier as such?
> also the idx < 0 maybe <1 ?
>
> Stiill needs some work...
>

BTW, nm fails to connect with
 <WARN>  nm_device_wifi_disable_encryption(): error setting key for
device wlan2: No such file or directory
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Johannes Berg]

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

On Tue, 2009-05-12 at 20:08 +0100, Hin-Tak Leung wrote:

> BTW, nm fails to connect with
>  <WARN>  nm_device_wifi_disable_encryption(): error setting key for
> device wlan2: No such file or directory

As Dan said, you can ignore this message. However, I'm certainly no
longer getting it. Are you sure you rebooted into the correct kernel and
loaded the correct cfg80211 module?

I have now tested WPA-PSK and WEP with wpa_supplicant and WEP with
manual configuration and it all works.

johannes

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

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Hin-Tak Leung]

On 5/12/09, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2009-05-12 at 20:05 +0100, Hin-Tak Leung wrote:
>
>> I tried this, NM/wpa_suplicany still doesn't work, but if I shut those
>> down and do it the old fashioned iwconfig way, I can get
>> connectivity...
>
> That's odd. Are you getting "connectivity" with a WEP network then?

Yes,


On 5/12/09, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2009-05-12 at 20:08 +0100, Hin-Tak Leung wrote:
>
>> BTW, nm fails to connect with
>>  <WARN>  nm_device_wifi_disable_encryption(): error setting key for
>> device wlan2: No such file or directory
>
> As Dan said, you can ignore this message. However, I'm certainly no
> longer getting it. Are you sure you rebooted into the correct kernel and
> loaded the correct cfg80211 module?

This is a different one from the last - it was 'Invalid argument' ,
now 'No such file or directory' - but wlan2 is certainly there...

I am operating in compat-wireless, so it is just make clean && make ,
etc, blow away /lib/module/<ver>/updates, depmod -a ; and I did run
modprobe -v to see it is loaded from the alternative...

> I have now tested WPA-PSK and WEP with wpa_supplicant and WEP with
> manual configuration and it all works.

I'll give it another try with NM/wpa_supplicant...

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Hin-Tak Leung]

> On 5/12/09, Johannes Berg <johannes@sipsolutions.net> wrote:
>> I have now tested WPA-PSK and WEP with wpa_supplicant and WEP with
>> manual configuration and it all works.

Tried again and it still won't work with NM/wpa_supplicant. FWIW, the
manual way I do it is this:

ifconfig wlan2 up
iwconfig wlan2 ESSID <apname>
iwconfig wlan2 key restrict <key>
iwconfig wlan2 AP <apip>
dhclient wlan2

killing dhclient then runs '/etc/rc.d/init.d/NetworkManager start'
usually switches back to nm-managed connection. (most of the same info
is stored in keyring, so nm-applet re-appears and starts spinning and
some seconds later I get the signal bars).

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Hin-Tak Leung]

Hiya, I stuck in a few printk(KERN_DEBUG __LINE__) around the new
-EINVAL's and tried to see why setting things by iwconfig manually
works, but NM/wpa_supplicant does not,  and here is what I found.
Around line 600 of net/wireless/wext-compat.c (this is the hackish mod
version):
------------------------------------------
int cfg80211_wext_siwencodeext(struct net_device *dev,
	                       struct iw_request_info *info,
                               struct iw_point *erq, char *extra)

        switch (ext->alg) {

        case IW_ENCODE_ALG_WEP:
                if (erq->length == 5)
                        cipher = WLAN_CIPHER_SUITE_WEP40;
                else if (erq->length == 13)
			cipher = WLAN_CIPHER_SUITE_WEP104;
                else {
                  printk(KERN_DEBUG "line %d %d\n", __LINE__, erq->length);
                        cipher = WLAN_CIPHER_SUITE_WEP104;
                        /* return -EINVAL; */
                 }
                break;
        }
------------------------------------------------
For some unknown reason, when run with NM/wpa_supplicant with the same
authentication credentials to the same AP, erq->length is 53 instead
of 13. If I just modify it as above instead of returning EINVAL, then
I get to authenticate, etc. in the old mac80211 ioctls, the decision
of cipher is postponed a lot later, after playing with the default key
a bit?

Anyway, I think 53 is either 40+13 or 13 *4 +1, so is it a case of
wpa_supplicant putting more stuff at the end or an offset somewhere?

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Johannes Berg]

Hi!

Thanks for the extensive debugging!

On Wed, 2009-05-13 at 04:56 +0100, Hin-Tak Leung wrote:
> Hiya, I stuck in a few printk(KERN_DEBUG __LINE__) around the new
> -EINVAL's and tried to see why setting things by iwconfig manually
> works, but NM/wpa_supplicant does not,  and here is what I found.
> Around line 600 of net/wireless/wext-compat.c (this is the hackish mod
> version):
> ------------------------------------------
> int cfg80211_wext_siwencodeext(struct net_device *dev,
> 	                       struct iw_request_info *info,
>                                struct iw_point *erq, char *extra)
> 
>         switch (ext->alg) {
> 
>         case IW_ENCODE_ALG_WEP:
>                 if (erq->length == 5)
>                         cipher = WLAN_CIPHER_SUITE_WEP40;
>                 else if (erq->length == 13)
> 			cipher = WLAN_CIPHER_SUITE_WEP104;
>                 else {
>                   printk(KERN_DEBUG "line %d %d\n", __LINE__, erq->length);
>                         cipher = WLAN_CIPHER_SUITE_WEP104;
>                         /* return -EINVAL; */
>                  }
>                 break;
>         }

Ok, so iwencodeext is used, presumably by wpa_supplicant because NM
never uses that ioctl, at least not as far as I can tell.

> ------------------------------------------------
> For some unknown reason, when run with NM/wpa_supplicant with the same
> authentication credentials to the same AP, erq->length is 53 instead
> of 13. 

That's strange. Do you know which wpa_supplicant version and NM you are
using? Is it always 53, or could it be random?

> If I just modify it as above instead of returning EINVAL, then
> I get to authenticate, etc. in the old mac80211 ioctls, the decision
> of cipher is postponed a lot later, after playing with the default key
> a bit?
> 
> Anyway, I think 53 is either 40+13 or 13 *4 +1, so is it a case of
> wpa_supplicant putting more stuff at the end or an offset somewhere?

No, that's sizeof(struct iw_encode_ext) and now I'm confused as to why
this actually worked for me. Ok, I see now I think, can you try this
patch?

johannes

--- wireless-testing.orig/net/wireless/wext-compat.c	2009-05-13 11:03:12.000000000 +0200
+++ wireless-testing/net/wireless/wext-compat.c	2009-05-13 11:03:35.000000000 +0200
@@ -614,9 +614,9 @@ int cfg80211_wext_siwencodeext(struct ne
 		cipher = 0;
 		break;
 	case IW_ENCODE_ALG_WEP:
-		if (erq->length == 5)
+		if (ext->key_len == 5)
 			cipher = WLAN_CIPHER_SUITE_WEP40;
-		else if (erq->length == 13)
+		else if (ext->key_len == 13)
 			cipher = WLAN_CIPHER_SUITE_WEP104;
 		else
 			return -EINVAL;

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Dan Williams]

On Wed, 2009-05-13 at 11:04 +0200, Johannes Berg wrote:
> Hi!
> 
> Thanks for the extensive debugging!
> 
> On Wed, 2009-05-13 at 04:56 +0100, Hin-Tak Leung wrote:
> > Hiya, I stuck in a few printk(KERN_DEBUG __LINE__) around the new
> > -EINVAL's and tried to see why setting things by iwconfig manually
> > works, but NM/wpa_supplicant does not,  and here is what I found.
> > Around line 600 of net/wireless/wext-compat.c (this is the hackish mod
> > version):
> > ------------------------------------------
> > int cfg80211_wext_siwencodeext(struct net_device *dev,
> > 	                       struct iw_request_info *info,
> >                                struct iw_point *erq, char *extra)
> > 
> >         switch (ext->alg) {
> > 
> >         case IW_ENCODE_ALG_WEP:
> >                 if (erq->length == 5)
> >                         cipher = WLAN_CIPHER_SUITE_WEP40;
> >                 else if (erq->length == 13)
> > 			cipher = WLAN_CIPHER_SUITE_WEP104;
> >                 else {
> >                   printk(KERN_DEBUG "line %d %d\n", __LINE__, erq->length);
> >                         cipher = WLAN_CIPHER_SUITE_WEP104;
> >                         /* return -EINVAL; */
> >                  }
> >                 break;
> >         }
> 
> Ok, so iwencodeext is used, presumably by wpa_supplicant because NM
> never uses that ioctl, at least not as far as I can tell.
> 
> > ------------------------------------------------
> > For some unknown reason, when run with NM/wpa_supplicant with the same
> > authentication credentials to the same AP, erq->length is 53 instead
> > of 13. 
> 
> That's strange. Do you know which wpa_supplicant version and NM you are
> using? Is it always 53, or could it be random?
> 
> > If I just modify it as above instead of returning EINVAL, then
> > I get to authenticate, etc. in the old mac80211 ioctls, the decision
> > of cipher is postponed a lot later, after playing with the default key
> > a bit?
> > 
> > Anyway, I think 53 is either 40+13 or 13 *4 +1, so is it a case of
> > wpa_supplicant putting more stuff at the end or an offset somewhere?
> 
> No, that's sizeof(struct iw_encode_ext) and now I'm confused as to why
> this actually worked for me. Ok, I see now I think, can you try this
> patch?
> 
> johannes
> 
> --- wireless-testing.orig/net/wireless/wext-compat.c	2009-05-13 11:03:12.000000000 +0200
> +++ wireless-testing/net/wireless/wext-compat.c	2009-05-13 11:03:35.000000000 +0200
> @@ -614,9 +614,9 @@ int cfg80211_wext_siwencodeext(struct ne
>  		cipher = 0;
>  		break;
>  	case IW_ENCODE_ALG_WEP:
> -		if (erq->length == 5)
> +		if (ext->key_len == 5)
>  			cipher = WLAN_CIPHER_SUITE_WEP40;
> -		else if (erq->length == 13)
> +		else if (ext->key_len == 13)
>  			cipher = WLAN_CIPHER_SUITE_WEP104;
>  		else
>  			return -EINVAL;

Yeah, you really do want ext->key_len there, not erq->length.
erq->length is the size of the whole WEXT request, not the key itself.

Dan

Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls

[Johannes Berg]

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

On Tue, 2009-05-12 at 20:05 +0100, Hin-Tak Leung wrote:

> I tried this, NM/wpa_suplicany still doesn't work, but if I shut those
> down and do it the old fashioned iwconfig way, I can get
> connectivity... 

That's odd. Are you getting "connectivity" with a WEP network then?

>  the last chunk looks a bit funny - '(idx < 4 || idx >
> 5)' is just '(idx != 4)' so why it isn't written simplier as such?
> also the idx < 0 maybe <1 ?

No? <4 || >5 is anything but 4 and 5. And idx<0 certainly is correct too
there.

johannes

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

[PATCH v2] cfg80211: fix a couple of bugs with key ioctls

[Johannes Berg]

There are a few small bugs/oversights in the key handling
code I wrote:
 * SIOCSIWENCODE should default to setting key 0 if no
   default key is set already and no index is given,
 * key removal should not require key material,
 * SIOCSIWENCODEEXT should default to changing the default
   management key if no index is given.
 * SIOCSIWENCODEEXT needs to use ext->key_len rather than
   erq->length to verify the key length

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/wireless/wext-compat.c |   18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

--- wireless-testing.orig/net/wireless/wext-compat.c	2009-05-12 12:38:41.000000000 +0200
+++ wireless-testing/net/wireless/wext-compat.c	2009-05-13 12:09:13.000000000 +0200
@@ -557,7 +557,7 @@ int cfg80211_wext_siwencode(struct net_d
 	if (idx == 0) {
 		idx = wdev->wext.default_key;
 		if (idx < 0)
-			return -EINVAL;
+			idx = 0;
 	} else if (idx < 1 || idx > 4)
 		return -EINVAL;
 	else
@@ -580,7 +580,7 @@ int cfg80211_wext_siwencode(struct net_d
 		params.cipher = WLAN_CIPHER_SUITE_WEP40;
 	else if (erq->length == 13)
 		params.cipher = WLAN_CIPHER_SUITE_WEP104;
-	else
+	else if (!remove)
 		return -EINVAL;
 
 	return cfg80211_set_encryption(rdev, dev, NULL, remove,
@@ -614,9 +614,9 @@ int cfg80211_wext_siwencodeext(struct ne
 		cipher = 0;
 		break;
 	case IW_ENCODE_ALG_WEP:
-		if (erq->length == 5)
+		if (ext->key_len == 5)
 			cipher = WLAN_CIPHER_SUITE_WEP40;
-		else if (erq->length == 13)
+		else if (ext->key_len == 13)
 			cipher = WLAN_CIPHER_SUITE_WEP104;
 		else
 			return -EINVAL;
@@ -640,13 +640,9 @@ int cfg80211_wext_siwencodeext(struct ne
 	idx = erq->flags & IW_ENCODE_INDEX;
 	if (cipher == WLAN_CIPHER_SUITE_AES_CMAC) {
 		if (idx < 4 || idx > 5) {
-			/*
-			 * XXX: Only wpa_supplicant ever used this
-			 *	can we still change the ABI a little
-			 *	so we do not need to keep track of
-			 *	the default key?
-			 */
-			return -EINVAL;
+			idx = wdev->wext.default_mgmt_key;
+			if (idx < 0)
+				return -EINVAL;
 		} else
 			idx--;
 	} else {

Re: [PATCH v2] cfg80211: fix a couple of bugs with key ioctls

[Hin-Tak Leung]

On Wed, May 13, 2009 at 11:10 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> There are a few small bugs/oversights in the key handling
> code I wrote:
> =A0* SIOCSIWENCODE should default to setting key 0 if no
> =A0 default key is set already and no index is given,
> =A0* key removal should not require key material,
> =A0* SIOCSIWENCODEEXT should default to changing the default
> =A0 management key if no index is given.
> =A0* SIOCSIWENCODEEXT needs to use ext->key_len rather than
> =A0 erq->length to verify the key length
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Tested-by: Hin-Tak Leung <htl10@users.sourceforge.net>  ... I think I
truly earned it this time :-). This works, and I also tried the
iwconfig way and that is still working. I was using an earlier
wpa_supplicant 0.6.8-1, but switched to 0.6.8-3 (
http://koji.fedoraproject.org/koji/buildinfo?buildID=3D101920 )
yesterday seeing as Dan has put some new stuff in.

The other question -, the number was always 53.

Thanks for fixing this.

Hin-Tak

> ---
> =A0net/wireless/wext-compat.c | =A0 18 +++++++-----------
> =A01 file changed, 7 insertions(+), 11 deletions(-)
>
> --- wireless-testing.orig/net/wireless/wext-compat.c =A0 =A02009-05-1=
2 12:38:41.000000000 +0200
> +++ wireless-testing/net/wireless/wext-compat.c 2009-05-13 12:09:13.0=
00000000 +0200
> @@ -557,7 +557,7 @@ int cfg80211_wext_siwencode(struct net_d
> =A0 =A0 =A0 =A0if (idx =3D=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0idx =3D wdev->wext.default_key;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (idx < 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 idx =3D 0;
> =A0 =A0 =A0 =A0} else if (idx < 1 || idx > 4)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> =A0 =A0 =A0 =A0else
> @@ -580,7 +580,7 @@ int cfg80211_wext_siwencode(struct net_d
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0params.cipher =3D WLAN_CIPHER_SUITE_WE=
P40;
> =A0 =A0 =A0 =A0else if (erq->length =3D=3D 13)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0params.cipher =3D WLAN_CIPHER_SUITE_WE=
P104;
> - =A0 =A0 =A0 else
> + =A0 =A0 =A0 else if (!remove)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
>
> =A0 =A0 =A0 =A0return cfg80211_set_encryption(rdev, dev, NULL, remove=
,
> @@ -614,9 +614,9 @@ int cfg80211_wext_siwencodeext(struct ne
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cipher =3D 0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0case IW_ENCODE_ALG_WEP:
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (erq->length =3D=3D 5)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ext->key_len =3D=3D 5)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cipher =3D WLAN_CIPHER=
_SUITE_WEP40;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 else if (erq->length =3D=3D 13)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 else if (ext->key_len =3D=3D 13)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cipher =3D WLAN_CIPHER=
_SUITE_WEP104;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> @@ -640,13 +640,9 @@ int cfg80211_wext_siwencodeext(struct ne
> =A0 =A0 =A0 =A0idx =3D erq->flags & IW_ENCODE_INDEX;
> =A0 =A0 =A0 =A0if (cipher =3D=3D WLAN_CIPHER_SUITE_AES_CMAC) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (idx < 4 || idx > 5) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* XXX: Only wpa_supp=
licant ever used this
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0can we =
still change the ABI a little
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0so we d=
o not need to keep track of
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* =A0 =A0 =A0the def=
ault key?
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 idx =3D wdev->wext.defa=
ult_mgmt_key;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (idx < 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return =
-EINVAL;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0idx--;
> =A0 =A0 =A0 =A0} else {
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html