Key information in wext.connect is not reset on (re)connect and can hold data from a previous connection.
Reset key data to avoid that drivers or mac80211 incorrectly detect a WEP connection request and access the freed or already reused memory.
Additionally optimize cfg80211_sme_connect() and avoid an useless schedule of conn_work.
Fixes: fffd0934b939 ("cfg80211: rework key operation") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/c80f04d2-8159-a02a-9287-26e5ec838826@wetzel-home.d... Signed-off-by: Alexander Wetzel alexander@wetzel-home.de
--- V2 changes: - updated comment - reset more key data
--- net/wireless/sme.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 123248b2c0be..0cc841c0c59b 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -285,6 +285,15 @@ void cfg80211_conn_work(struct work_struct *work) wiphy_unlock(&rdev->wiphy); }
+static void cfg80211_step_auth_next(struct cfg80211_conn *conn, + struct cfg80211_bss *bss) +{ + memcpy(conn->bssid, bss->bssid, ETH_ALEN); + conn->params.bssid = conn->bssid; + conn->params.channel = bss->channel; + conn->state = CFG80211_CONN_AUTHENTICATE_NEXT; +} + /* Returned bss is reference counted and must be cleaned up appropriately. */ static struct cfg80211_bss *cfg80211_get_conn_bss(struct wireless_dev *wdev) { @@ -302,10 +311,7 @@ static struct cfg80211_bss *cfg80211_get_conn_bss(struct wireless_dev *wdev) if (!bss) return NULL;
- memcpy(wdev->conn->bssid, bss->bssid, ETH_ALEN); - wdev->conn->params.bssid = wdev->conn->bssid; - wdev->conn->params.channel = bss->channel; - wdev->conn->state = CFG80211_CONN_AUTHENTICATE_NEXT; + cfg80211_step_auth_next(wdev->conn, bss); schedule_work(&rdev->conn_work);
return bss; @@ -597,7 +603,12 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev, wdev->conn->params.ssid_len = wdev->u.client.ssid_len;
/* see if we have the bss already */ - bss = cfg80211_get_conn_bss(wdev); + bss = cfg80211_get_bss(wdev->wiphy, wdev->conn->params.channel, + wdev->conn->params.bssid, + wdev->conn->params.ssid, + wdev->conn->params.ssid_len, + wdev->conn_bss_type, + IEEE80211_PRIVACY(wdev->conn->params.privacy));
if (prev_bssid) { memcpy(wdev->conn->prev_bssid, prev_bssid, ETH_ALEN); @@ -608,6 +619,7 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev, if (bss) { enum nl80211_timeout_reason treason;
+ cfg80211_step_auth_next(wdev->conn, bss); err = cfg80211_conn_do_work(wdev, &treason); cfg80211_put_bss(wdev->wiphy, bss); } else { @@ -1464,6 +1476,15 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev, } else { if (WARN_ON(connkeys)) return -EINVAL; + + /* connect can point to wdev->wext.connect which + * can hold key data from a previous connection + */ + connect->key = NULL; + connect->key_len = 0; + connect->key_idx = 0; + connect->crypto.cipher_group = 0; + connect->crypto.n_ciphers_pairwise = 0; }
wdev->connect_keys = connkeys;
Hi,
This broke WPA auth entirely on brcmfmac (in offload mode) and probably others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please revert or fix. Notes below.
Reported-by: Ilya me@0upti.me Reported-by: Janne Grunau j@jannau.net
#regzbot introduced: 015b8cc5e7c4d7 #regzbot monitor: https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wet...
On 24/01/2023 23.18, Alexander Wetzel wrote:
Key information in wext.connect is not reset on (re)connect and can hold data from a previous connection.
Reset key data to avoid that drivers or mac80211 incorrectly detect a WEP connection request and access the freed or already reused memory.
Additionally optimize cfg80211_sme_connect() and avoid an useless schedule of conn_work.
Fixes: fffd0934b939 ("cfg80211: rework key operation") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/c80f04d2-8159-a02a-9287-26e5ec838826@wetzel-home.d... Signed-off-by: Alexander Wetzel alexander@wetzel-home.de
V2 changes:
- updated comment
- reset more key data
net/wireless/sme.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 123248b2c0be..0cc841c0c59b 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c
[snip]
@@ -1464,6 +1476,15 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev,
This if branch only fires if the connection is WEP.
} else { if (WARN_ON(connkeys)) return -EINVAL;
/* connect can point to wdev->wext.connect which* can hold key data from a previous connection*/connect->key = NULL;connect->key_len = 0;connect->key_idx = 0;
And these are indeed only used by WEP.
connect->crypto.cipher_group = 0;connect->crypto.n_ciphers_pairwise = 0;
But here you're killing the info that is used for *other* auth modes too if !WEP, breaking WPA and everything else.
} wdev->connect_keys = connkeys;
- Hector
Hi Hector,
On 3/11/23 10:55, Hector Martin wrote:
Hi,
This broke WPA auth entirely on brcmfmac (in offload mode) and probably others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please revert or fix. Notes below.
Reported-by: Ilya me@0upti.me Reported-by: Janne Grunau j@jannau.net
#regzbot introduced: 015b8cc5e7c4d7 #regzbot monitor: https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wet...
I can confirm this bug, I was seeing broken wifi on brcmfmac with 6.3-rc1 and I was about to start a git bisect for this this morning when I saw this email.
Reverting 015b8cc5e7c4d7 fixes the broken wifi. Hector, thank you, you just saved me from a bisect on somewhat slow hardware :)
Regards,
Hans
On 24/01/2023 23.18, Alexander Wetzel wrote:
Key information in wext.connect is not reset on (re)connect and can hold data from a previous connection.
Reset key data to avoid that drivers or mac80211 incorrectly detect a WEP connection request and access the freed or already reused memory.
Additionally optimize cfg80211_sme_connect() and avoid an useless schedule of conn_work.
Fixes: fffd0934b939 ("cfg80211: rework key operation") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/c80f04d2-8159-a02a-9287-26e5ec838826@wetzel-home.d... Signed-off-by: Alexander Wetzel alexander@wetzel-home.de
V2 changes:
- updated comment
- reset more key data
net/wireless/sme.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 123248b2c0be..0cc841c0c59b 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c
[snip]
@@ -1464,6 +1476,15 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev,
This if branch only fires if the connection is WEP.
} else { if (WARN_ON(connkeys)) return -EINVAL;
/* connect can point to wdev->wext.connect which* can hold key data from a previous connection*/connect->key = NULL;connect->key_len = 0;connect->key_idx = 0;And these are indeed only used by WEP.
connect->crypto.cipher_group = 0;connect->crypto.n_ciphers_pairwise = 0;But here you're killing the info that is used for *other* auth modes too if !WEP, breaking WPA and everything else.
} wdev->connect_keys = connkeys;
- Hector
On Sat, Mar 11, 2023 at 12:03:44PM +0100, Hans de Goede wrote:
Hi Hector,
On 3/11/23 10:55, Hector Martin wrote:
Hi,
This broke WPA auth entirely on brcmfmac (in offload mode) and probably others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please revert or fix. Notes below.
Reported-by: Ilya me@0upti.me Reported-by: Janne Grunau j@jannau.net
#regzbot introduced: 015b8cc5e7c4d7 #regzbot monitor: https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wet...
I can confirm this bug, I was seeing broken wifi on brcmfmac with 6.3-rc1 and I was about to start a git bisect for this this morning when I saw this email.
Reverting 015b8cc5e7c4d7 fixes the broken wifi. Hector, thank you, you just saved me from a bisect on somewhat slow hardware :)
Great, can someone submit the revert patch to the networking tree so we can get this resolved quickly?
thanks,
greg k-h
This reverts part of commit 015b8cc5e7c4 ("wifi: cfg80211: Fix use after free for wext")
This commit broke WPA offload by unconditionally clearing the crypto modes for non-WEP connections. Drop that part of the patch.
Fixes: 015b8cc5e7c4 ("wifi: cfg80211: Fix use after free for wext") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/linux-wireless/ZAx0TWRBlGfv7pNl@kroah.com/T/#m11e6e0... Signed-off-by: Hector Martin marcan@marcan.st --- net/wireless/sme.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 28ce13840a88..7bdeb8eea92d 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -1500,8 +1500,6 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev, connect->key = NULL; connect->key_len = 0; connect->key_idx = 0; - connect->crypto.cipher_group = 0; - connect->crypto.n_ciphers_pairwise = 0; }
wdev->connect_keys = connkeys; -- 2.35.1
On 2023-03-11 23:19:14 +0900, Hector Martin wrote:
This reverts part of commit 015b8cc5e7c4 ("wifi: cfg80211: Fix use after free for wext")
This commit broke WPA offload by unconditionally clearing the crypto modes for non-WEP connections. Drop that part of the patch.
Fixes: 015b8cc5e7c4 ("wifi: cfg80211: Fix use after free for wext") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/linux-wireless/ZAx0TWRBlGfv7pNl@kroah.com/T/#m11e6e0... Signed-off-by: Hector Martin marcan@marcan.st
net/wireless/sme.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 28ce13840a88..7bdeb8eea92d 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -1500,8 +1500,6 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev, connect->key = NULL; connect->key_len = 0; connect->key_idx = 0;
connect->crypto.cipher_group = 0;connect->crypto.n_ciphers_pairwise = 0;}
wdev->connect_keys = connkeys;
Tested-by: Janne Grunau j@jannau.net
thanks,
Janne
On Sat, 11 Mar 2023 at 14:28, Hector Martin marcan@marcan.st wrote:
This reverts part of commit 015b8cc5e7c4 ("wifi: cfg80211: Fix use after free for wext")
This commit broke WPA offload by unconditionally clearing the crypto modes for non-WEP connections. Drop that part of the patch.
Fixes: 015b8cc5e7c4 ("wifi: cfg80211: Fix use after free for wext") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/linux-wireless/ZAx0TWRBlGfv7pNl@kroah.com/T/#m11e6e0... Signed-off-by: Hector Martin marcan@marcan.st
Reviewed-by: Eric Curtin ecurtin@redhat.com
Is mise le meas/Regards,
Eric Curtin
net/wireless/sme.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 28ce13840a88..7bdeb8eea92d 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -1500,8 +1500,6 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev, connect->key = NULL; connect->key_len = 0; connect->key_idx = 0;
connect->crypto.cipher_group = 0;connect->crypto.n_ciphers_pairwise = 0; } wdev->connect_keys = connkeys;-- 2.35.1
[TLDR: This mail in primarily relevant for Linux kernel regression tracking. See link in footer if these mails annoy you.]
On 11.03.23 10:55, Hector Martin wrote:
This broke WPA auth entirely on brcmfmac (in offload mode) and probably others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please revert or fix. Notes below.
Reported-by: Ilya me@0upti.me Reported-by: Janne Grunau j@jannau.net
#regzbot introduced: 015b8cc5e7c4d7 #regzbot monitor: https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wet...
#regzbot fix: 79d1ed5ca7db67d48 #regzbot ignore-activity
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
linux-stable-mirror@lists.linaro.org
-
Alexander Wetzel -
Eric Curtin -
Greg Kroah-Hartman -
Hans de Goede -
Hector Martin -
Janne Grunau -
Linux regression tracking #update (Thorsten Leemhuis)