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: [email protected]
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Alexander Wetzel <[email protected]>
---
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;
--
2.39.0
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 <[email protected]>
Reported-by: Janne Grunau <[email protected]>
#regzbot introduced: 015b8cc5e7c4d7
#regzbot monitor:
https://lore.kernel.org/linux-wireless/[email protected]/
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: [email protected]
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Alexander Wetzel <[email protected]>
>
> ---
> 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 <[email protected]>
> Reported-by: Janne Grunau <[email protected]>
>
> #regzbot introduced: 015b8cc5e7c4d7
> #regzbot monitor:
> https://lore.kernel.org/linux-wireless/[email protected]/
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: [email protected]
>> Link: https://lore.kernel.org/r/[email protected]
>> Signed-off-by: Alexander Wetzel <[email protected]>
>>
>> ---
>> 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 <[email protected]>
> > Reported-by: Janne Grunau <[email protected]>
> >
> > #regzbot introduced: 015b8cc5e7c4d7
> > #regzbot monitor:
> > https://lore.kernel.org/linux-wireless/[email protected]/
>
> 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
I can also confirm the regression with the (out-of-tree) broadcom-wl driver.
As Hector said reverting the cipher_group / n_ciphers_pairwise lines is enough.
See: https://gist.github.com/joanbm/fb90f4807b719a2e37a496936faabec9
Regards,
- Joan
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: [email protected]
Link: https://lore.kernel.org/linux-wireless/[email protected]/T/#m11e6e0915ab8fa19ce8bc9695ab288c0fe018edf
Signed-off-by: Hector Martin <[email protected]>
---
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: [email protected]
> Link: https://lore.kernel.org/linux-wireless/[email protected]/T/#m11e6e0915ab8fa19ce8bc9695ab288c0fe018edf
> Signed-off-by: Hector Martin <[email protected]>
> ---
> 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 <[email protected]>
thanks,
Janne
On Sat, 11 Mar 2023 at 14:28, Hector Martin <[email protected]> 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: [email protected]
> Link: https://lore.kernel.org/linux-wireless/[email protected]/T/#m11e6e0915ab8fa19ce8bc9695ab288c0fe018edf
> Signed-off-by: Hector Martin <[email protected]>
Reviewed-by: Eric Curtin <[email protected]>
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
>
>
Ok, sorry forgot to add some CCs. Beside the reported affected kernels
by Hector, who also provided the fix, it seems the appointed patch
affects all stable releases as shown below.
On 12.03.23 22:36, Greg Kroah-Hartman wrote:
> On Sun, Mar 12, 2023 at 10:11:29PM +0700, Philip Müller wrote:
>> I see,
>>
>> however, seems we have a full house here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/releases/4.14.308/wifi-cfg80211-fix-use-after-free-for-wext.patch
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/releases/4.19.276/wifi-cfg80211-fix-use-after-free-for-wext.patch
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/releases/5.4.235/wifi-cfg80211-fix-use-after-free-for-wext.patch
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/releases/5.10.173/wifi-cfg80211-fix-use-after-free-for-wext.patch
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/releases/5.15.99/wifi-cfg80211-fix-use-after-free-for-wext.patch
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/releases/6.1.16/wifi-cfg80211-fix-use-after-free-for-wext.patch
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/releases/6.2.3/wifi-cfg80211-fix-use-after-free-for-wext.patch
>
> I do not understand, sorry.
>
> Also, why is this a private message? Kernel development is done in
> public please.
>
> thanks,
>
> greg k-h
--
Best, Philip
[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 <[email protected]>
> Reported-by: Janne Grunau <[email protected]>
>
> #regzbot introduced: 015b8cc5e7c4d7
> #regzbot monitor:
> https://lore.kernel.org/linux-wireless/[email protected]/
#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.