[PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[shanyu.zhao]

From: Shanyu Zhao <shanyu.zhao@intel.com>

The cfg80211's SIOCSIWAP function is broken, the reason is we cannot
get the correct bss with previously set ssid. Need to clear it by
setting ssid_len to 0 in cfg80211_mgd_wext_siwap() and fix other
places accordingly.

Cc: stable@kernel.org

Signed-off-by: Shanyu Zhao <shanyu.zhao@intel.com>
---
v2: Adding Cc to stable@kernel.org
v3: set bssid to NULL in cfg80211_mgd_wext_siwessid, otherwise after
    iwconfig wlanx ap <mac> the iwconfig wlanx essid won't work.
---
 net/wireless/sme.c      |    9 +++++++--
 net/wireless/wext-sme.c |    4 +++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 745c37e..51c3cdb 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -779,10 +779,15 @@ int __cfg80211_connect(struct cfg80211_registered_device *rdev,
 			wdev->conn->auto_auth = false;
 		}
 
-		memcpy(wdev->ssid, connect->ssid, connect->ssid_len);
+		/* make sure it can connect if only bssid is provided */
 		wdev->ssid_len = connect->ssid_len;
-		wdev->conn->params.ssid = wdev->ssid;
 		wdev->conn->params.ssid_len = connect->ssid_len;
+		if (connect->ssid_len) {
+			memcpy(wdev->ssid, connect->ssid, connect->ssid_len);
+			wdev->conn->params.ssid = wdev->ssid;
+		}
+		else
+			wdev->conn->params.ssid = NULL;
 
 		/* see if we have the bss already */
 		bss = cfg80211_get_conn_bss(wdev);
diff --git a/net/wireless/wext-sme.c b/net/wireless/wext-sme.c
index 5615a88..5a67dd5 100644
--- a/net/wireless/wext-sme.c
+++ b/net/wireless/wext-sme.c
@@ -34,7 +34,7 @@ int cfg80211_mgd_wext_connect(struct cfg80211_registered_device *rdev,
 			wdev->wext.connect.privacy = true;
 	}
 
-	if (!wdev->wext.connect.ssid_len)
+	if (!wdev->wext.connect.ssid_len && !wdev->wext.connect.bssid)
 		return 0;
 
 	if (wdev->wext.keys) {
@@ -194,6 +194,7 @@ int cfg80211_mgd_wext_siwessid(struct net_device *dev,
 	wdev->wext.connect.ssid = wdev->wext.ssid;
 	memcpy(wdev->wext.ssid, ssid, len);
 	wdev->wext.connect.ssid_len = len;
+	wdev->wext.connect.bssid = NULL;
 
 	wdev->wext.connect.crypto.control_port = false;
 
@@ -280,6 +281,7 @@ int cfg80211_mgd_wext_siwap(struct net_device *dev,
 	if (bssid) {
 		memcpy(wdev->wext.bssid, bssid, ETH_ALEN);
 		wdev->wext.connect.bssid = wdev->wext.bssid;
+		wdev->wext.connect.ssid_len = 0;
 	} else
 		wdev->wext.connect.bssid = NULL;
 
-- 
1.6.0.4

Re: [PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[John W. Linville]

On Fri, Jan 29, 2010 at 01:47:29PM -0800, shanyu.zhao@intel.com wrote:
> From: Shanyu Zhao <shanyu.zhao@intel.com>
> 
> The cfg80211's SIOCSIWAP function is broken, the reason is we cannot
> get the correct bss with previously set ssid. Need to clear it by
> setting ssid_len to 0 in cfg80211_mgd_wext_siwap() and fix other
> places accordingly.
> 
> Cc: stable@kernel.org
> 
> Signed-off-by: Shanyu Zhao <shanyu.zhao@intel.com>
> ---
> v2: Adding Cc to stable@kernel.org
> v3: set bssid to NULL in cfg80211_mgd_wext_siwessid, otherwise after
>     iwconfig wlanx ap <mac> the iwconfig wlanx essid won't work.

I already merged v2.  Please send the difference between v2 and v3
as a separate patch.

Thanks,

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[Johannes Berg]

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


> @@ -779,10 +779,15 @@ int __cfg80211_connect(struct cfg80211_registered_device *rdev,
>  			wdev->conn->auto_auth = false;
>  		}
>  
> -		memcpy(wdev->ssid, connect->ssid, connect->ssid_len);
> +		/* make sure it can connect if only bssid is provided */
>  		wdev->ssid_len = connect->ssid_len;
> -		wdev->conn->params.ssid = wdev->ssid;
>  		wdev->conn->params.ssid_len = connect->ssid_len;
> +		if (connect->ssid_len) {
> +			memcpy(wdev->ssid, connect->ssid, connect->ssid_len);
> +			wdev->conn->params.ssid = wdev->ssid;
> +		}
> +		else
> +			wdev->conn->params.ssid = NULL;

I don't think I understand this. The wdev->conn->params are initialised
from "connect", and now you're copying the data back again? That's not
making sense to me.

Also, please say what exactly are you trying to fix.

Also, cfg80211_mgd_wext_siwap() certainly is _not_ expected to clear the
SSID, since we want to connect to the previously set SSID. Same with
cfg80211_mgd_wext_siwessid(), it should _not_ clear the BSSID.

Right now, it looks to me that your patch makes it impossible to specify
both the SSID and the BSSID to connect to, which is definitely wrong.

John, you mentioned you had applied this, can you revert this patch for
now?

johannes

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

Re: [PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[Johannes Berg]

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

On Sat, 2010-01-30 at 09:35 +0100, Johannes Berg wrote:

> >  		wdev->conn->params.ssid_len = connect->ssid_len;
> > +		if (connect->ssid_len) {
> > +			memcpy(wdev->ssid, connect->ssid, connect->ssid_len);
> > +			wdev->conn->params.ssid = wdev->ssid;
> > +		}
> > +		else
> > +			wdev->conn->params.ssid = NULL;
> 
> I don't think I understand this. The wdev->conn->params are initialised
> from "connect", and now you're copying the data back again? That's not
> making sense to me.

Hmm, so setting the ssid to NULL would make it impossible to connect to
any network, no?

I don't think I understand what you want to achieve. Are you, by any
chance, trying to get it to connect to an AP without ever specifying an
SSID? That can't really be supported (nor ever was by mac80211).

johannes

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

RE: [PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[Zhao, Shanyu]

WWVzLCB0aGF0J3MgZXhhY3RseSB3aGF0IEkgdHJpZWQgdG8gZG8uIEZyb20gd2hhdCBJIHJlYWQg
ZnJvbSB0aGUgbWFuIHBhZ2Ugb2YgaXdjb25maWcsIEkgdGhvdWdodCBpd2NvbmZpZyB3bGFueCBh
cCA8bWFjPiBzaG91bGQgYmUgZW5vdWdoIHRvIGNvbm5lY3QgdG8gYW4gYXAgd2l0aG91dCBzcGVj
aWZ5aW5nIHRoZSBlc3NpZC4gTWF5YmUgSSBkaWRuJ3QgdW5kZXJzdGFuZCB0aGUgY29ycmVjdCB1
c2FnZSBvZiBpd2NvbmZpZy4NCg0KSG93ZXZlciwgb25lIG9mIG91ciB0ZXN0IGNhc2VzIGZhaWxl
ZCBiZWNhdXNlIG9mIHRoaXM6DQpTdGVwcyB0byBSZXByb2R1Y2U6IA0KMS4gc2V0IHVwIHRoZSBT
U0lEIG5hbWUgd2l0aCBPcGVuL05vbmUgaW4gQVANCjIuIHJlbG9hZCB0aGUgZHJpdmVyDQozLiBp
bnRlcmZhY2UgdXANCjQuIHNlbmQgdGhlIGNvbW1hbmQgJ2l3Y29uZmlnIHdsYW4wIGFwIDxNQUMg
YWRkcmVzcyBvZiB0aGUgQVA+DQoNCkV4cGVjdGVkIFJlc3VsdHM6DQp0aGUgYXNzb2NpYXRpb24g
aXMgZXN0YWJsaXNoZWQgc3VjY2Vzc2Z1bGx5Lg0KDQpUaGFua3MsDQpTaGFueXUNCg0KLS0tLS1P
cmlnaW5hbCBNZXNzYWdlLS0tLS0NCkZyb206IEpvaGFubmVzIEJlcmcgW21haWx0bzpqb2hhbm5l
c0BzaXBzb2x1dGlvbnMubmV0XSANClNlbnQ6IFNhdHVyZGF5LCBKYW51YXJ5IDMwLCAyMDEwIDEy
OjQ0IEFNDQpUbzogWmhhbywgU2hhbnl1DQpDYzogbGludXgtd2lyZWxlc3NAdmdlci5rZXJuZWwu
b3JnOyBsaW52aWxsZUB0dXhkcml2ZXIuY29tDQpTdWJqZWN0OiBSZTogW1BBVENIIDEvMSB2M10g
Y2ZnODAyMTE6IHdleHQ6IGZpeCBjZmc4MDIxMSdzIFNJT0NTSVdBUCBmdW5jdGlvbg0KDQpPbiBT
YXQsIDIwMTAtMDEtMzAgYXQgMDk6MzUgKzAxMDAsIEpvaGFubmVzIEJlcmcgd3JvdGU6DQoNCj4g
PiAgCQl3ZGV2LT5jb25uLT5wYXJhbXMuc3NpZF9sZW4gPSBjb25uZWN0LT5zc2lkX2xlbjsNCj4g
PiArCQlpZiAoY29ubmVjdC0+c3NpZF9sZW4pIHsNCj4gPiArCQkJbWVtY3B5KHdkZXYtPnNzaWQs
IGNvbm5lY3QtPnNzaWQsIGNvbm5lY3QtPnNzaWRfbGVuKTsNCj4gPiArCQkJd2Rldi0+Y29ubi0+
cGFyYW1zLnNzaWQgPSB3ZGV2LT5zc2lkOw0KPiA+ICsJCX0NCj4gPiArCQllbHNlDQo+ID4gKwkJ
CXdkZXYtPmNvbm4tPnBhcmFtcy5zc2lkID0gTlVMTDsNCj4gDQo+IEkgZG9uJ3QgdGhpbmsgSSB1
bmRlcnN0YW5kIHRoaXMuIFRoZSB3ZGV2LT5jb25uLT5wYXJhbXMgYXJlIGluaXRpYWxpc2VkDQo+
IGZyb20gImNvbm5lY3QiLCBhbmQgbm93IHlvdSdyZSBjb3B5aW5nIHRoZSBkYXRhIGJhY2sgYWdh
aW4/IFRoYXQncyBub3QNCj4gbWFraW5nIHNlbnNlIHRvIG1lLg0KDQpIbW0sIHNvIHNldHRpbmcg
dGhlIHNzaWQgdG8gTlVMTCB3b3VsZCBtYWtlIGl0IGltcG9zc2libGUgdG8gY29ubmVjdCB0bw0K
YW55IG5ldHdvcmssIG5vPw0KDQpJIGRvbid0IHRoaW5rIEkgdW5kZXJzdGFuZCB3aGF0IHlvdSB3
YW50IHRvIGFjaGlldmUuIEFyZSB5b3UsIGJ5IGFueQ0KY2hhbmNlLCB0cnlpbmcgdG8gZ2V0IGl0
IHRvIGNvbm5lY3QgdG8gYW4gQVAgd2l0aG91dCBldmVyIHNwZWNpZnlpbmcgYW4NClNTSUQ/IFRo
YXQgY2FuJ3QgcmVhbGx5IGJlIHN1cHBvcnRlZCAobm9yIGV2ZXIgd2FzIGJ5IG1hYzgwMjExKS4N
Cg0Kam9oYW5uZXMNCg==

RE: [PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[Johannes Berg]

On Sat, 2010-01-30 at 21:24 -0800, Zhao, Shanyu wrote:
> Yes, that's exactly what I tried to do. From what I read from the man
> page of iwconfig, I thought iwconfig wlanx ap <mac> should be enough
> to connect to an ap without specifying the essid. Maybe I didn't
> understand the correct usage of iwconfig.
> 
> However, one of our test cases failed because of this:
> Steps to Reproduce: 
> 1. set up the SSID name with Open/None in AP
> 2. reload the driver
> 3. interface up
> 4. send the command 'iwconfig wlan0 ap <MAC address of the AP>
> 
> Expected Results:
> the association is established successfully.

Fun, so you ran into one of the few things we don't (want to) support.
And to be honest, I don't think there's a good reason to support it; for
example this would never connect to a hidden SSID network.

johannes

Re: [PATCH 1/1 v3] cfg80211: wext: fix cfg80211's SIOCSIWAP function

[John W. Linville]

On Sat, Jan 30, 2010 at 09:35:45AM +0100, Johannes Berg wrote:
> 
> > @@ -779,10 +779,15 @@ int __cfg80211_connect(struct cfg80211_registered_device *rdev,
> >  			wdev->conn->auto_auth = false;
> >  		}
> >  
> > -		memcpy(wdev->ssid, connect->ssid, connect->ssid_len);
> > +		/* make sure it can connect if only bssid is provided */
> >  		wdev->ssid_len = connect->ssid_len;
> > -		wdev->conn->params.ssid = wdev->ssid;
> >  		wdev->conn->params.ssid_len = connect->ssid_len;
> > +		if (connect->ssid_len) {
> > +			memcpy(wdev->ssid, connect->ssid, connect->ssid_len);
> > +			wdev->conn->params.ssid = wdev->ssid;
> > +		}
> > +		else
> > +			wdev->conn->params.ssid = NULL;
> 
> I don't think I understand this. The wdev->conn->params are initialised
> from "connect", and now you're copying the data back again? That's not
> making sense to me.
> 
> Also, please say what exactly are you trying to fix.
> 
> Also, cfg80211_mgd_wext_siwap() certainly is _not_ expected to clear the
> SSID, since we want to connect to the previously set SSID. Same with
> cfg80211_mgd_wext_siwessid(), it should _not_ clear the BSSID.
> 
> Right now, it looks to me that your patch makes it impossible to specify
> both the SSID and the BSSID to connect to, which is definitely wrong.
> 
> John, you mentioned you had applied this, can you revert this patch for
> now?

Yeah, it's coming out...  Clearly I didn't understand what it was doing.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.