[PATCH v2] wifi: cfg80211: Fix use after free for wext

[PATCH v2] wifi: cfg80211: Fix use after free for wext

[Alexander Wetzel]

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.de
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;
-- 
2.39.0

[REGRESSION] Patch broke WPA auth: Re: [PATCH v2] wifi: cfg80211: Fix use after free for wext

[Hector Martin]

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@wetzel-home.de/

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.de
> 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

Re: [REGRESSION] Patch broke WPA auth: Re: [PATCH v2] wifi: cfg80211: Fix use after free for wext

[Hans de Goede]

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@wetzel-home.de/

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.de
>> 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
> 

Re: [REGRESSION] Patch broke WPA auth: Re: [PATCH v2] wifi: cfg80211: Fix use after free for wext

[Greg Kroah-Hartman]

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@wetzel-home.de/
> 
> 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

[PATCH] wifi: cfg80211: Partial revert "wifi: cfg80211: Fix use after free for wext"

[Hector Martin]

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/#m11e6e0915ab8fa19ce8bc9695ab288c0fe018edf
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

Re: [PATCH] wifi: cfg80211: Partial revert "wifi: cfg80211: Fix use after free for wext"

[Janne Grunau]

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/#m11e6e0915ab8fa19ce8bc9695ab288c0fe018edf
> 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

Re: [PATCH] wifi: cfg80211: Partial revert "wifi: cfg80211: Fix use after free for wext"

[Eric Curtin]

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/#m11e6e0915ab8fa19ce8bc9695ab288c0fe018edf
> 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
>
>

Re: [REGRESSION] Patch broke WPA auth: Re: [PATCH v2] wifi: cfg80211: Fix use after free for wext

[Linux regression tracking #update (Thorsten Leemhuis)]

[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@wetzel-home.de/

#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.

Re: [REGRESSION] Patch broke WPA auth: Re: [PATCH v2] wifi: cfg80211: Fix use after free for wext

[Philip Müller]

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