| Message ID | 20231204155558.133839-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap() | expand |
Dmitry Antipov <dmantipov@yandex.ru> writes: > Since 'ieee80211_beacon_get()' can return NULL, 'wfx_set_mfp_ap()' > should check the return value before examining skb data. So convert > the latter to return an appropriate error code and propagate it to > return from 'wfx_start_ap()' as well. Compile tested only. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> Patches like this should be tested on a real device.
Hello Dmitry, On Monday 4 December 2023 16:55:37 CET Dmitry Antipov wrote: > > Since 'ieee80211_beacon_get()' can return NULL, 'wfx_set_mfp_ap()' > should check the return value before examining skb data. So convert > the latter to return an appropriate error code and propagate it to > return from 'wfx_start_ap()' as well. Compile tested only. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > drivers/net/wireless/silabs/wfx/sta.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c > index 1b6c158457b4..df100d8513ad 100644 > --- a/drivers/net/wireless/silabs/wfx/sta.c > +++ b/drivers/net/wireless/silabs/wfx/sta.c > @@ -336,29 +336,35 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif) > return 0; > } > > -static void wfx_set_mfp_ap(struct wfx_vif *wvif) > +static int 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); > + const u16 *ptr; > > + if (unlikely(!skb)) > + return -ENOMEM; > + > + ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset, > + skb->len - ieoffset); > if (ptr) { The code would be slightly better if we would invert this condition: if (!ptr) return -EINVAL; > ptr += pairwise_cipher_suite_count_offset; > if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb))) > - return; > + return -EINVAL; > ptr += 1 + pairwise_cipher_suite_size * *ptr; > if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb))) > - return; > + return -EINVAL; > ptr += 1 + akm_suite_size * *ptr; > if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb))) > - return; > + return -EINVAL; > wfx_hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6)); > + return 0; > } > + return -EINVAL; > } > > int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > @@ -374,10 +380,7 @@ int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > wvif = (struct wfx_vif *)vif->drv_priv; > wfx_upload_ap_templates(wvif); > ret = wfx_hif_start(wvif, &vif->bss_conf, wvif->channel); > - if (ret > 0) > - return -EIO; > - wfx_set_mfp_ap(wvif); > - return ret; > + return ret > 0 ? -EIO : wfx_set_mfp_ap(wvif); I would prefer to not abuse of the trinary operator. I would prefer: if (ret > 0) return -EIO; return wfx_set_mfp_ap(wvif); > } > > void wfx_stop_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > -- > 2.43.0 > > I agree with the patch. Could you fix the cosmetics issues? I will take care of testing it on real hardware.
diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c index 1b6c158457b4..df100d8513ad 100644 --- a/drivers/net/wireless/silabs/wfx/sta.c +++ b/drivers/net/wireless/silabs/wfx/sta.c @@ -336,29 +336,35 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif) return 0; } -static void wfx_set_mfp_ap(struct wfx_vif *wvif) +static int 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); + const u16 *ptr; + if (unlikely(!skb)) + return -ENOMEM; + + ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset, + skb->len - ieoffset); if (ptr) { ptr += pairwise_cipher_suite_count_offset; if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb))) - return; + return -EINVAL; ptr += 1 + pairwise_cipher_suite_size * *ptr; if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb))) - return; + return -EINVAL; ptr += 1 + akm_suite_size * *ptr; if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb))) - return; + return -EINVAL; wfx_hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6)); + return 0; } + return -EINVAL; } int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif, @@ -374,10 +380,7 @@ int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif, wvif = (struct wfx_vif *)vif->drv_priv; wfx_upload_ap_templates(wvif); ret = wfx_hif_start(wvif, &vif->bss_conf, wvif->channel); - if (ret > 0) - return -EIO; - wfx_set_mfp_ap(wvif); - return ret; + return ret > 0 ? -EIO : wfx_set_mfp_ap(wvif); } void wfx_stop_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
Since 'ieee80211_beacon_get()' can return NULL, 'wfx_set_mfp_ap()' should check the return value before examining skb data. So convert the latter to return an appropriate error code and propagate it to return from 'wfx_start_ap()' as well. Compile tested only. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- drivers/net/wireless/silabs/wfx/sta.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)