[PATCH] wifi: wfx: repair open network AP mode

[PATCH] wifi: wfx: repair open network AP mode

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

RSN IE missing in beacon is normal in open networks.
Avoid returning -ENODEV in this case.

Steps to reproduce:

$ cat /etc/wpa_supplicant.conf
network={
	ssid="testNet"
	mode=2
	key_mgmt=NONE
}

$ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
nl80211: Beacon set failed: -22 (Invalid argument)
Failed to set beacon parameters
Interface initialization failed
wlan0: interface state UNINITIALIZED->DISABLED
wlan0: AP-DISABLED
wlan0: Unable to setup interface.
Failed to initialize AP interface

After the change:

$ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
Successfully initialized wpa_supplicant
wlan0: interface state UNINITIALIZED->ENABLED
wlan0: AP-ENABLED

Cc: stable@vger.kernel.org
Fixes: fe0a7776d4d1 ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
 drivers/net/wireless/silabs/wfx/sta.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
index 216d43c8bd6e..7c04810dbf3d 100644
--- a/drivers/net/wireless/silabs/wfx/sta.c
+++ b/drivers/net/wireless/silabs/wfx/sta.c
@@ -352,8 +352,11 @@ static int wfx_set_mfp_ap(struct wfx_vif *wvif)
 
 	ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
 				      skb->len - ieoffset);
-	if (unlikely(!ptr))
+	if (!ptr) {
+		/* No RSN IE is fine in open networks */
+		ret = 0;
 		goto free_skb;
+	}
 
 	ptr += pairwise_cipher_suite_count_offset;
 	if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
-- 
2.46.0

Re: [PATCH] wifi: wfx: repair open network AP mode

[Sverdlin, Alexander]

Hi!

On Fri, 2024-08-23 at 15:15 +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> RSN IE missing in beacon is normal in open networks.
> Avoid returning -ENODEV in this case.

Oops, this is a typo and should have been "-EINVAL".
I'm ready to respin with corrected commit message, but
I'm also OK with a maintainer massaging it.

> 
> Steps to reproduce:
> 
> $ cat /etc/wpa_supplicant.conf
> network={
> 	ssid="testNet"
> 	mode=2
> 	key_mgmt=NONE
> }
> 
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> nl80211: Beacon set failed: -22 (Invalid argument)
> Failed to set beacon parameters
> Interface initialization failed
> wlan0: interface state UNINITIALIZED->DISABLED
> wlan0: AP-DISABLED
> wlan0: Unable to setup interface.
> Failed to initialize AP interface
> 
> After the change:
> 
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> Successfully initialized wpa_supplicant
> wlan0: interface state UNINITIALIZED->ENABLED
> wlan0: AP-ENABLED
> 
> Cc: stable@vger.kernel.org
> Fixes: fe0a7776d4d1 ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
>  drivers/net/wireless/silabs/wfx/sta.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
> index 216d43c8bd6e..7c04810dbf3d 100644
> --- a/drivers/net/wireless/silabs/wfx/sta.c
> +++ b/drivers/net/wireless/silabs/wfx/sta.c
> @@ -352,8 +352,11 @@ static int wfx_set_mfp_ap(struct wfx_vif *wvif)
>  
>  	ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
>  				      skb->len - ieoffset);
> -	if (unlikely(!ptr))
> +	if (!ptr) {
> +		/* No RSN IE is fine in open networks */
> +		ret = 0;
>  		goto free_skb;
> +	}
>  
>  	ptr += pairwise_cipher_suite_count_offset;
>  	if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH] wifi: wfx: repair open network AP mode

[Kalle Valo]

"Sverdlin, Alexander" <alexander.sverdlin@siemens.com> writes:

> Hi!
>
> On Fri, 2024-08-23 at 15:15 +0200, A. Sverdlin wrote:
>> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>> 
>> RSN IE missing in beacon is normal in open networks.
>> Avoid returning -ENODEV in this case.
>
> Oops, this is a typo and should have been "-EINVAL".
> I'm ready to respin with corrected commit message, but
> I'm also OK with a maintainer massaging it.

I can fix the commit message, no need to resend because of this.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html

Re: [PATCH] wifi: wfx: repair open network AP mode

[Kalle Valo]

"A. Sverdlin" <alexander.sverdlin@siemens.com> writes:

> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>
> RSN IE missing in beacon is normal in open networks.
> Avoid returning -ENODEV in this case.
>
> Steps to reproduce:
>
> $ cat /etc/wpa_supplicant.conf
> network={
> 	ssid="testNet"
> 	mode=2
> 	key_mgmt=NONE
> }
>
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> nl80211: Beacon set failed: -22 (Invalid argument)
> Failed to set beacon parameters
> Interface initialization failed
> wlan0: interface state UNINITIALIZED->DISABLED
> wlan0: AP-DISABLED
> wlan0: Unable to setup interface.
> Failed to initialize AP interface
>
> After the change:
>
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> Successfully initialized wpa_supplicant
> wlan0: interface state UNINITIALIZED->ENABLED
> wlan0: AP-ENABLED

BTW excellent commit message, immediately obvious what was the problem
and how it was tested. I wish everyone would do the same.

> Cc: stable@vger.kernel.org
> Fixes: fe0a7776d4d1 ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

I think this should go to wireless tree for v6.11, right?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html

Re: [PATCH] wifi: wfx: repair open network AP mode

[Sverdlin, Alexander]

Hi!

On Fri, 2024-08-23 at 18:07 +0300, Kalle Valo wrote:
> > RSN IE missing in beacon is normal in open networks.
> > Avoid returning -ENODEV in this case.
> > 
> > Steps to reproduce:
> > 
> > $ cat /etc/wpa_supplicant.conf
> > network={
> >  	ssid="testNet"
> >  	mode=2
> >  	key_mgmt=NONE
> > }
> > 
> > $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> > nl80211: Beacon set failed: -22 (Invalid argument)
> > Failed to set beacon parameters
> > Interface initialization failed
> > wlan0: interface state UNINITIALIZED->DISABLED
> > wlan0: AP-DISABLED
> > wlan0: Unable to setup interface.
> > Failed to initialize AP interface
> > 
> > After the change:
> > 
> > $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> > Successfully initialized wpa_supplicant
> > wlan0: interface state UNINITIALIZED->ENABLED
> > wlan0: AP-ENABLED
> 
> BTW excellent commit message, immediately obvious what was the problem
> and how it was tested. I wish everyone would do the same.

Thanks!

> > Cc: stable@vger.kernel.org
> > Fixes: fe0a7776d4d1 ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()")
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> I think this should go to wireless tree for v6.11, right?

Makes sense to me! Sorry, I've missed the proper tagging!
Whatever makes it into stable. I've already tested v6.1 and v6.8 where it applies
as-is.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH] wifi: wfx: repair open network AP mode

[Jérôme Pouiller]

On Friday 23 August 2024 15:15:20 CEST A. Sverdlin wrote:
> 
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> RSN IE missing in beacon is normal in open networks.
> Avoid returning -ENODEV in this case.
> 
> Steps to reproduce:
> 
> $ cat /etc/wpa_supplicant.conf
> network={
>         ssid="testNet"
>         mode=2
>         key_mgmt=NONE
> }
> 
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> nl80211: Beacon set failed: -22 (Invalid argument)
> Failed to set beacon parameters
> Interface initialization failed
> wlan0: interface state UNINITIALIZED->DISABLED
> wlan0: AP-DISABLED
> wlan0: Unable to setup interface.
> Failed to initialize AP interface
> 
> After the change:
> 
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> Successfully initialized wpa_supplicant
> wlan0: interface state UNINITIALIZED->ENABLED
> wlan0: AP-ENABLED

Good catch, thank you.

> 
> Cc: stable@vger.kernel.org
> Fixes: fe0a7776d4d1 ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
>  drivers/net/wireless/silabs/wfx/sta.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
> index 216d43c8bd6e..7c04810dbf3d 100644
> --- a/drivers/net/wireless/silabs/wfx/sta.c
> +++ b/drivers/net/wireless/silabs/wfx/sta.c
> @@ -352,8 +352,11 @@ static int wfx_set_mfp_ap(struct wfx_vif *wvif)
> 
>         ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
>                                       skb->len - ieoffset);
> -       if (unlikely(!ptr))
> +       if (!ptr) {
> +               /* No RSN IE is fine in open networks */
> +               ret = 0;
>                 goto free_skb;
> +       }
> 
>         ptr += pairwise_cipher_suite_count_offset;
>         if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))

wfx_hif_set_mfp() is no more called when open network is started. Normally,
wfx_hif_reset() is sufficient to avoid any side effect with previous calls
to wfx_hif_set_mfp().

However, if you don't mind, I would prefer to call wfx_hif_set_mfp() in all
cases.

-- 
Jérôme Pouiller

Re: [PATCH] wifi: wfx: repair open network AP mode

[Sverdlin, Alexander]

Hello Jérôme!

Thank you for the quick reply!

On Mon, 2024-08-26 at 17:12 +0200, Jérôme Pouiller wrote:
> On Friday 23 August 2024 15:15:20 CEST A. Sverdlin wrote:
> > 
> > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > RSN IE missing in beacon is normal in open networks.
> > Avoid returning -ENODEV in this case.
> > 
> > Steps to reproduce:
> > 
> > $ cat /etc/wpa_supplicant.conf
> > network={
> >          ssid="testNet"
> >          mode=2
> >          key_mgmt=NONE
> > }
> > 
> > $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> > nl80211: Beacon set failed: -22 (Invalid argument)
> > Failed to set beacon parameters
> > Interface initialization failed
> > wlan0: interface state UNINITIALIZED->DISABLED
> > wlan0: AP-DISABLED
> > wlan0: Unable to setup interface.
> > Failed to initialize AP interface
> > 
> > After the change:
> > 
> > $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> > Successfully initialized wpa_supplicant
> > wlan0: interface state UNINITIALIZED->ENABLED
> > wlan0: AP-ENABLED
> 
> Good catch, thank you.
> 
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: fe0a7776d4d1 ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()")
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > ---
> >   drivers/net/wireless/silabs/wfx/sta.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
> > index 216d43c8bd6e..7c04810dbf3d 100644
> > --- a/drivers/net/wireless/silabs/wfx/sta.c
> > +++ b/drivers/net/wireless/silabs/wfx/sta.c
> > @@ -352,8 +352,11 @@ static int wfx_set_mfp_ap(struct wfx_vif *wvif)
> > 
> >          ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
> >                                        skb->len - ieoffset);
> > -       if (unlikely(!ptr))
> > +       if (!ptr) {
> > +               /* No RSN IE is fine in open networks */
> > +               ret = 0;
> >                  goto free_skb;
> > +       }
> > 
> >          ptr += pairwise_cipher_suite_count_offset;
> >          if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
> 
> wfx_hif_set_mfp() is no more called when open network is started. Normally,
> wfx_hif_reset() is sufficient to avoid any side effect with previous calls
> to wfx_hif_set_mfp().
> 
> However, if you don't mind, I would prefer to call wfx_hif_set_mfp() in all
> cases.

I'm a little bit confused by this comment... You write "wfx_hif_set_mfp() is no more called",
but I struggle to find when it was last time called (for open networks).
Not when you visited this part of the code in commit b8cfb7c819dd
("wifi: wfx: fix memory leak when starting AP"), not in fe0a7776d4d1
("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()").
And even not before the latter change (say, fe0a7776d4d1^):

static void wfx_set_mfp_ap(struct wfx_vif *wvif)
{
	struct ieee80211_vif *vif = wvif_to_vif(wvif);
	struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, vif, 0);
	const int ieoffset = offsetof(struct ieee80211_mgmt, u.beacon.variable);
	const u16 *ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
						 skb->len - ieoffset);
	const int pairwise_cipher_suite_count_offset = 8 / sizeof(u16);
	const int pairwise_cipher_suite_size = 4 / sizeof(u16);
	const int akm_suite_size = 4 / sizeof(u16);

	if (ptr) {
		ptr += pairwise_cipher_suite_count_offset;
		if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
			return;
		ptr += 1 + pairwise_cipher_suite_size * *ptr;
		if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
			return;
		ptr += 1 + akm_suite_size * *ptr;
		if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
			return;
		wfx_hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6));
	}
}

What do I miss?

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH] wifi: wfx: repair open network AP mode

[Jérôme Pouiller]

On Monday 26 August 2024 17:42:28 CEST Sverdlin, Alexander wrote:
[...]
> On Mon, 2024-08-26 at 17:12 +0200, Jérôme Pouiller wrote:
> > On Friday 23 August 2024 15:15:20 CEST A. Sverdlin wrote:
> > >
> > > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

[...]

> >
> > wfx_hif_set_mfp() is no more called when open network is started. Normally,
> > wfx_hif_reset() is sufficient to avoid any side effect with previous calls
> > to wfx_hif_set_mfp().
> >
> > However, if you don't mind, I would prefer to call wfx_hif_set_mfp() in all
> > cases.
> 
> I'm a little bit confused by this comment... You write "wfx_hif_set_mfp() is no more called",
> but I struggle to find when it was last time called (for open networks).
> Not when you visited this part of the code in commit b8cfb7c819dd
> ("wifi: wfx: fix memory leak when starting AP"), not in fe0a7776d4d1
> ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()").
> And even not before the latter change (say, fe0a7776d4d1^):
> 
> static void wfx_set_mfp_ap(struct wfx_vif *wvif)
> {
>         struct ieee80211_vif *vif = wvif_to_vif(wvif);
>         struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, vif, 0);
>         const int ieoffset = offsetof(struct ieee80211_mgmt, u.beacon.variable);
>         const u16 *ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
>                                                  skb->len - ieoffset);
>         const int pairwise_cipher_suite_count_offset = 8 / sizeof(u16);
>         const int pairwise_cipher_suite_size = 4 / sizeof(u16);
>         const int akm_suite_size = 4 / sizeof(u16);
> 
>         if (ptr) {
>                 ptr += pairwise_cipher_suite_count_offset;
>                 if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
>                         return;
>                 ptr += 1 + pairwise_cipher_suite_size * *ptr;
>                 if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
>                         return;
>                 ptr += 1 + akm_suite_size * *ptr;
>                 if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
>                         return;
>                 wfx_hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6));
>         }
> }
> 
> What do I miss?

Indeed, you're right. This was the original behavior. So:

Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>


-- 
Jérôme Pouiller

Re: wifi: wfx: repair open network AP mode

[Kalle Valo]

"A. Sverdlin" <alexander.sverdlin@siemens.com> wrote:

> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> RSN IE missing in beacon is normal in open networks.
> Avoid returning -EINVAL in this case.
> 
> Steps to reproduce:
> 
> $ cat /etc/wpa_supplicant.conf
> network={
> 	ssid="testNet"
> 	mode=2
> 	key_mgmt=NONE
> }
> 
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> nl80211: Beacon set failed: -22 (Invalid argument)
> Failed to set beacon parameters
> Interface initialization failed
> wlan0: interface state UNINITIALIZED->DISABLED
> wlan0: AP-DISABLED
> wlan0: Unable to setup interface.
> Failed to initialize AP interface
> 
> After the change:
> 
> $ wpa_supplicant -iwlan0 -c /etc/wpa_supplicant.conf
> Successfully initialized wpa_supplicant
> wlan0: interface state UNINITIALIZED->ENABLED
> wlan0: AP-ENABLED
> 
> Cc: stable@vger.kernel.org
> Fixes: fe0a7776d4d1 ("wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

Patch applied to wireless.git, thanks.

6d30bb88f623 wifi: wfx: repair open network AP mode

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240823131521.3309073-1-alexander.sverdlin@siemens.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html