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

[Dan Williams <dcbw@redhat.com>]

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