2009-05-12 10:45:10

by Johannes Berg

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

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 <[email protected]>
---
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 {




2009-05-12 21:00:51

by Hin-Tak Leung

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

> On 5/12/09, Johannes Berg <[email protected]> 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).

2009-05-13 10:11:04

by Johannes Berg

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

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 <[email protected]>
---
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 {



2009-05-12 19:21:09

by Johannes Berg

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

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


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2009-05-12 19:59:29

by Hin-Tak Leung

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

On 5/12/09, Johannes Berg <[email protected]> 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 <[email protected]> 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...

2009-05-12 19:28:19

by Johannes Berg

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

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


Attachments:
signature.asc (801.00 B)
This is a digitally signed message part

2009-05-13 13:27:18

by Hin-Tak Leung

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

On Wed, May 13, 2009 at 11:10 AM, Johannes Berg
<[email protected]> 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 <[email protected]>

Tested-by: Hin-Tak Leung <[email protected]> ... 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 {
>
>
>

2009-05-12 19:08:23

by Hin-Tak Leung

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

On Tue, May 12, 2009 at 8:05 PM, Hin-Tak Leung <[email protected]>=
wrote:
> On Tue, May 12, 2009 at 11:44 AM, Johannes Berg
> <[email protected]> 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 <[email protected]>
>> ---
>> =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

2009-05-13 09:05:16

by Johannes Berg

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

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;



2009-05-13 03:56:42

by Hin-Tak Leung

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

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?

2009-05-13 13:23:59

by Dan Williams

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

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



2009-05-12 19:05:51

by Hin-Tak Leung

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

On Tue, May 12, 2009 at 11:44 AM, Johannes Berg
<[email protected]> 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 <[email protected]>
> ---
> =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...