Re: [PATCH] wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()

["Jérôme Pouiller" <jerome.pouiller@silabs.com>]

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.



-- 
Jérôme Pouiller