| Message ID | 20240823131521.3309073-1-alexander.sverdlin@siemens.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6d30bb88f623526197c0e18a366e68a4254a2c83 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: wfx: repair open network AP mode | expand |
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)))
"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.
"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?
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.
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.
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?
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>
"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
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)))