* [PATCH] cfg80211: fix a couple of bugs with key ioctls
@ 2009-05-12 10:44 Johannes Berg
2009-05-12 19:05 ` Hin-Tak Leung
2009-05-13 10:10 ` [PATCH v2] " Johannes Berg
0 siblings, 2 replies; 12+ messages in thread
From: Johannes Berg @ 2009-05-12 10:44 UTC (permalink / raw)
To: John Linville; +Cc: linux-wireless, Hin-Tak Leung
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 {
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 10:44 [PATCH] cfg80211: fix a couple of bugs with key ioctls Johannes Berg
@ 2009-05-12 19:05 ` Hin-Tak Leung
2009-05-12 19:08 ` Hin-Tak Leung
2009-05-12 19:20 ` Johannes Berg
2009-05-13 10:10 ` [PATCH v2] " Johannes Berg
1 sibling, 2 replies; 12+ messages in thread
From: Hin-Tak Leung @ 2009-05-12 19:05 UTC (permalink / raw)
To: Johannes Berg; +Cc: John Linville, linux-wireless
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 19:05 ` Hin-Tak Leung
@ 2009-05-12 19:08 ` Hin-Tak Leung
2009-05-12 19:27 ` Johannes Berg
2009-05-12 19:20 ` Johannes Berg
1 sibling, 1 reply; 12+ messages in thread
From: Hin-Tak Leung @ 2009-05-12 19:08 UTC (permalink / raw)
To: Johannes Berg; +Cc: John Linville, linux-wireless
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 19:05 ` Hin-Tak Leung
2009-05-12 19:08 ` Hin-Tak Leung
@ 2009-05-12 19:20 ` Johannes Berg
1 sibling, 0 replies; 12+ messages in thread
From: Johannes Berg @ 2009-05-12 19:20 UTC (permalink / raw)
To: Hin-Tak Leung; +Cc: John Linville, linux-wireless, dcbw
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 19:08 ` Hin-Tak Leung
@ 2009-05-12 19:27 ` Johannes Berg
2009-05-12 19:59 ` Hin-Tak Leung
0 siblings, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2009-05-12 19:27 UTC (permalink / raw)
To: Hin-Tak Leung; +Cc: John Linville, linux-wireless, Dan Williams
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 19:27 ` Johannes Berg
@ 2009-05-12 19:59 ` Hin-Tak Leung
2009-05-12 21:00 ` Hin-Tak Leung
0 siblings, 1 reply; 12+ messages in thread
From: Hin-Tak Leung @ 2009-05-12 19:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: John Linville, linux-wireless, Dan Williams
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...
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 19:59 ` Hin-Tak Leung
@ 2009-05-12 21:00 ` Hin-Tak Leung
2009-05-13 3:56 ` Hin-Tak Leung
0 siblings, 1 reply; 12+ messages in thread
From: Hin-Tak Leung @ 2009-05-12 21:00 UTC (permalink / raw)
To: Johannes Berg; +Cc: John Linville, linux-wireless, Dan Williams
> 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).
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 21:00 ` Hin-Tak Leung
@ 2009-05-13 3:56 ` Hin-Tak Leung
2009-05-13 9:04 ` Johannes Berg
0 siblings, 1 reply; 12+ messages in thread
From: Hin-Tak Leung @ 2009-05-13 3:56 UTC (permalink / raw)
To: Johannes Berg; +Cc: John Linville, linux-wireless, Dan Williams
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-13 3:56 ` Hin-Tak Leung
@ 2009-05-13 9:04 ` Johannes Berg
2009-05-13 13:24 ` Dan Williams
0 siblings, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2009-05-13 9:04 UTC (permalink / raw)
To: Hin-Tak Leung; +Cc: John Linville, linux-wireless, Dan Williams, Jouni Malinen
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;
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] cfg80211: fix a couple of bugs with key ioctls
2009-05-12 10:44 [PATCH] cfg80211: fix a couple of bugs with key ioctls Johannes Berg
2009-05-12 19:05 ` Hin-Tak Leung
@ 2009-05-13 10:10 ` Johannes Berg
2009-05-13 13:27 ` Hin-Tak Leung
1 sibling, 1 reply; 12+ messages in thread
From: Johannes Berg @ 2009-05-13 10:10 UTC (permalink / raw)
To: John Linville; +Cc: linux-wireless, Hin-Tak Leung
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 {
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] cfg80211: fix a couple of bugs with key ioctls
2009-05-13 9:04 ` Johannes Berg
@ 2009-05-13 13:24 ` Dan Williams
0 siblings, 0 replies; 12+ messages in thread
From: Dan Williams @ 2009-05-13 13:24 UTC (permalink / raw)
To: Johannes Berg; +Cc: Hin-Tak Leung, John Linville, linux-wireless, Jouni Malinen
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cfg80211: fix a couple of bugs with key ioctls
2009-05-13 10:10 ` [PATCH v2] " Johannes Berg
@ 2009-05-13 13:27 ` Hin-Tak Leung
0 siblings, 0 replies; 12+ messages in thread
From: Hin-Tak Leung @ 2009-05-13 13:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: John Linville, linux-wireless
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2009-05-13 13:27 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-12 10:44 [PATCH] cfg80211: fix a couple of bugs with key ioctls Johannes Berg
2009-05-12 19:05 ` Hin-Tak Leung
2009-05-12 19:08 ` Hin-Tak Leung
2009-05-12 19:27 ` Johannes Berg
2009-05-12 19:59 ` Hin-Tak Leung
2009-05-12 21:00 ` Hin-Tak Leung
2009-05-13 3:56 ` Hin-Tak Leung
2009-05-13 9:04 ` Johannes Berg
2009-05-13 13:24 ` Dan Williams
2009-05-12 19:20 ` Johannes Berg
2009-05-13 10:10 ` [PATCH v2] " Johannes Berg
2009-05-13 13:27 ` Hin-Tak Leung
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.