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

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

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