* [PATCH] wil6210: disallow changing RSN in beacon change
@ 2017-10-17 19:42 Johannes Berg
2017-10-18 9:25 ` Lior David
2017-10-27 13:42 ` Kalle Valo
0 siblings, 2 replies; 6+ messages in thread
From: Johannes Berg @ 2017-10-17 19:42 UTC (permalink / raw)
To: linux-wireless; +Cc: Maya Erez, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
This is a code path that will never really get hit anyway, since
it's nonsense to change the beacon of an existing BSS to suddenly
include or no longer include the RSN IE. Reject this instead of
having the dead code, and get rid of accessing wdev->ssid/_len by
way of that.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
drivers/net/wireless/ath/wil6210/cfg80211.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
index 85d5c04618eb..a81711451691 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -1389,7 +1389,6 @@ static int wil_cfg80211_change_beacon(struct wiphy *wiphy,
struct cfg80211_beacon_data *bcon)
{
struct wil6210_priv *wil = wiphy_to_wil(wiphy);
- int rc;
u32 privacy = 0;
wil_dbg_misc(wil, "change_beacon\n");
@@ -1400,24 +1399,11 @@ static int wil_cfg80211_change_beacon(struct wiphy *wiphy,
bcon->tail_len))
privacy = 1;
- /* in case privacy has changed, need to restart the AP */
- if (wil->privacy != privacy) {
- struct wireless_dev *wdev = ndev->ieee80211_ptr;
-
- wil_dbg_misc(wil, "privacy changed %d=>%d. Restarting AP\n",
- wil->privacy, privacy);
-
- rc = _wil_cfg80211_start_ap(wiphy, ndev, wdev->ssid,
- wdev->ssid_len, privacy,
- wdev->beacon_interval,
- wil->channel, bcon,
- wil->hidden_ssid,
- wil->pbss);
- } else {
- rc = _wil_cfg80211_set_ies(wiphy, bcon);
- }
+ /* privacy (really RSN) shouldn't be changing */
+ if (wil->privacy != privacy)
+ return -EINVAL;
- return rc;
+ return _wil_cfg80211_set_ies(wiphy, bcon);
}
static int wil_cfg80211_start_ap(struct wiphy *wiphy,
--
2.14.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] wil6210: disallow changing RSN in beacon change
2017-10-17 19:42 [PATCH] wil6210: disallow changing RSN in beacon change Johannes Berg
@ 2017-10-18 9:25 ` Lior David
2017-10-18 10:13 ` Johannes Berg
2017-10-27 13:42 ` Kalle Valo
1 sibling, 1 reply; 6+ messages in thread
From: Lior David @ 2017-10-18 9:25 UTC (permalink / raw)
To: Johannes Berg, linux-wireless; +Cc: Maya Erez, Johannes Berg
Hi Johannes,
On 10/17/2017 10:42 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> This is a code path that will never really get hit anyway, since
> it's nonsense to change the beacon of an existing BSS to suddenly
> include or no longer include the RSN IE. Reject this instead of
> having the dead code, and get rid of accessing wdev->ssid/_len by
> way of that.
>
This is not dead code, we reach it in several scenarios, mainly WPS tests.
hostapd uses change_beacon to change the security of the AP so this needs to
be supported. We do need to restart the AP in this case which will disconnect
existing clients, but this cannot be helped...
As a side note, hostapd can also use change_beacon to change the SSID. It
does so by updating the SSID IE in the probe response frame. We have a pending
patch that detects this and updates the FW but we also need to update wdev->ssid
otherwise the wireless_dev will be out of date (not sure if it will cause
any problems...)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] wil6210: disallow changing RSN in beacon change
2017-10-18 9:25 ` Lior David
@ 2017-10-18 10:13 ` Johannes Berg
2017-10-19 6:07 ` Lior David
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2017-10-18 10:13 UTC (permalink / raw)
To: Lior David, linux-wireless; +Cc: Maya Erez, Jouni Malinen
Hi,
> This is not dead code, we reach it in several scenarios, mainly WPS
> tests.
Interesting.
> hostapd uses change_beacon to change the security of the AP so this
> needs to be supported.
I didn't think this made sense - Jouni? Does hostapd kick off all
stations in this case?
> We do need to restart the AP in this case which will
> disconnect existing clients, but this cannot be helped...
Why not restart the AP entirely then from userspace? Hmm. I wonder what
would happen with mac80211 - I guess keys would have to removed etc?
Does this just work by accident because mac80211 removes the keys with
stations? What about GTK(s) though?
> As a side note, hostapd can also use change_beacon to change the
> SSID.
When does that happen?
> It does so by updating the SSID IE in the probe response frame. We
> have a pending patch that detects this and updates the FW but we also
> need to update wdev->ssid otherwise the wireless_dev will be out of
> date (not sure if it will cause any problems...)
Logic-wise it won't, but we do expose this to userspace and that'd be
confusing, so we have to update it I guess.
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] wil6210: disallow changing RSN in beacon change
2017-10-18 10:13 ` Johannes Berg
@ 2017-10-19 6:07 ` Lior David
0 siblings, 0 replies; 6+ messages in thread
From: Lior David @ 2017-10-19 6:07 UTC (permalink / raw)
To: Johannes Berg, linux-wireless; +Cc: Maya Erez, Jouni Malinen
On 10/18/2017 1:13 PM, Johannes Berg wrote:
....
>> hostapd uses change_beacon to change the security of the AP so this
>> needs to be supported.
>
> I didn't think this made sense - Jouni? Does hostapd kick off all
> stations in this case?
>
>> We do need to restart the AP in this case which will
>> disconnect existing clients, but this cannot be helped...
>
> Why not restart the AP entirely then from userspace? Hmm. I wonder what
> would happen with mac80211 - I guess keys would have to removed etc?
> Does this just work by accident because mac80211 removes the keys with
> stations? What about GTK(s) though?
>
Not sure what happens when the privacy stays the same (secure) but keys
change, maybe Jouni can comment.
>> As a side note, hostapd can also use change_beacon to change the
>> SSID.
>
> When does that happen?
By chance I worked on a WPS certification test last week which used
a shell script to perform various operations. The AP started secure
but the script could change its configuration to unsecure. It used
the wps_config CLI command to change both the security and
SSID and hostapd used change_beacon to perform this operation.
We got this script from WIFI team so there is good chance it
is in use by existing certification test beds.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: wil6210: disallow changing RSN in beacon change
2017-10-17 19:42 [PATCH] wil6210: disallow changing RSN in beacon change Johannes Berg
2017-10-18 9:25 ` Lior David
@ 2017-10-27 13:42 ` Kalle Valo
2017-10-27 13:45 ` Johannes Berg
1 sibling, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2017-10-27 13:42 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Maya Erez, Johannes Berg
Johannes Berg <johannes@sipsolutions.net> wrote:
> This is a code path that will never really get hit anyway, since
> it's nonsense to change the beacon of an existing BSS to suddenly
> include or no longer include the RSN IE. Reject this instead of
> having the dead code, and get rid of accessing wdev->ssid/_len by
> way of that.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
What's the conclusion, should I take this patch or drop it?
--
https://patchwork.kernel.org/patch/10012789/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: wil6210: disallow changing RSN in beacon change
2017-10-27 13:42 ` Kalle Valo
@ 2017-10-27 13:45 ` Johannes Berg
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Berg @ 2017-10-27 13:45 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Maya Erez
On Fri, 2017-10-27 at 15:42 +0200, Kalle Valo wrote:
> Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > This is a code path that will never really get hit anyway, since
> > it's nonsense to change the beacon of an existing BSS to suddenly
> > include or no longer include the RSN IE. Reject this instead of
> > having the dead code, and get rid of accessing wdev->ssid/_len by
> > way of that.
> >
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
>
> What's the conclusion, should I take this patch or drop it?
Drop, at least for now. I need to look into this and probably do things
in cfg80211.
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-27 13:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-17 19:42 [PATCH] wil6210: disallow changing RSN in beacon change Johannes Berg
2017-10-18 9:25 ` Lior David
2017-10-18 10:13 ` Johannes Berg
2017-10-19 6:07 ` Lior David
2017-10-27 13:42 ` Kalle Valo
2017-10-27 13:45 ` Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).