Received-SPF: pass (google.com: domain of linux-wireless+bounces-384-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: =?ISO-8859-1?Q?J=E9r=F4me?= Pouiller <jerome.pouiller@silabs.com>
To: Dmitry Antipov <dmantipov@yandex.ru>
Cc: Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
 Dmitry Antipov <dmantipov@yandex.ru>
Subject:
 Re: [PATCH] wifi: wfx: fix possible NULL pointer dereference in
 wfx_set_mfp_ap()
Date: Mon, 04 Dec 2023 17:50:50 +0100
Message-ID: <4726634.8F6SAcFxjW@pc-42>
Organization: Silicon Labs
In-Reply-To: <20231204155558.133839-1-dmantipov@yandex.ru>
References: <20231204155558.133839-1-dmantipov@yandex.ru>
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?iso-8859-1?Q?jsoMwswRy9fqBIdG5qV4BOlpdjLq79x+kZAzoH2OqLZyNG6Ao3HF0q8zcL?=
 =?iso-8859-1?Q?3aQcqRT8w1Hb41vysv1Q8WyuGE+YIEuOxfVR40G1o2XL7NxTmlgUkULvg4?=
 =?iso-8859-1?Q?ieP7IVmZl3Wi6jpQrgG57h0jirQ0h6xBGMJKNH8SQyaBS50eEU5NnV6CFk?=
 =?iso-8859-1?Q?GLZxq3N7dIaZ0bWud9WwBphv+CAlqcmW7abuUoYu8SSQu8rWI73WUUnZPc?=
 =?iso-8859-1?Q?QdGCrhDUU9zu35vWcc4k1hvQ8XKquhD+p/luc+pHTL2CwLM0f4Q3WPftGi?=
 =?iso-8859-1?Q?Qq/FiyGBoDeQ8zlJITwcohcoBnusbTkN5umqebG1pHYjUcKnORnLEjZr9C?=
 =?iso-8859-1?Q?YswenvLW+oe+rdarqD+c1n4Erw5xhJ30FoHBrHt8QUosPLb3NYpzwJNlbf?=
 =?iso-8859-1?Q?8a9P8gd7TrXjFsntnxeZxP6hp9sMFrmfxlSDvANTywWexMv6PrOrysepQu?=
 =?iso-8859-1?Q?GcSm/3fGEwf4FNI42pF3WZjsJVFr1GmcUZ/RqvKryUYWulpeG9ftfJjTC2?=
 =?iso-8859-1?Q?2Th/7kZir+YfxCZNvIwOts8UCSzd9IG1ZryTnn0P0b1pVYcRRV0dNff09S?=
 =?iso-8859-1?Q?xLPkF5y0lAcCS7kCLyMpmygG6PHdPLn2TRAK6ILQQzmIbwJW7Yzd/SeW3q?=
 =?iso-8859-1?Q?Y7soPA6ggnPz0t8ETBOncCT92ZnAb6zc3V73gSm80fg8Z6B/jTt6+SFiOi?=
 =?iso-8859-1?Q?mMQaaExWXoeypHzudrDoEnXRxH/syiLIpaJgrc7MqH80Upa3ckNX8MWvxq?=
 =?iso-8859-1?Q?II6ebu+fsGLEsAbz86/mkaUSJjlaBqNqviMNcq/Z74GlzzbgY73pkCUEcG?=
 =?iso-8859-1?Q?z9tAtFtFpwP8YDzqVBYsOj8mqPNxt05KxpqHEE+aOeFRwcc+7DrYmdKNxn?=
 =?iso-8859-1?Q?Ikv0mXMjgOe6R/XdkWJNsPsQJIVVcuz9xEWMaPq6sJ7BSAtqLwAjlVdC3C?=
 =?iso-8859-1?Q?YSZ4TtbUND4MVZRfPK7jtKEgg3g+EzmPkJroJrG55vEgw1f5z4XuBQJBH4?=
 =?iso-8859-1?Q?bghmliuQtWfHtX/CQvRaBENi36liY5AT+l+meU7GJBxIV2eorfNwx1ERLx?=
 =?iso-8859-1?Q?tpjLM07yotgvDt35D3vxAKFCXwLIi/h9zIDLJeLTLxKwpIdKt6+MFmZAU4?=
 =?iso-8859-1?Q?o/145T5qdjZXvD30SctFobPDOKsHCDla6x5Iwa4A0qUgqXHo5tdjLYkJRP?=
 =?iso-8859-1?Q?tKlCjHMQvafj0/H17kwjkTLklLeMdjlkgb6jNmhD+lgbub4lOWdFyDmCpK?=
 =?iso-8859-1?Q?j5xl/k5hbWkHD7cCPKHsfijW8vC4AKUULqxF7r3cx/qr5ll4829C1J6yqp?=
 =?iso-8859-1?Q?/mdgPg7qtRF15GgoWAeQmPEo+zw5sLRorOk7PP5SD0hD3jJdR31RNmDyXD?=
 =?iso-8859-1?Q?4DcmOjOKWSdQiQxWhsOK44dOKnUAZ/+l8qLAR2os9ti/9nWSTQQBP9g1JE?=
 =?iso-8859-1?Q?PeXMMmIH5CjGQwFuprtgk9xsnleHrU0tGYO+SuncH5FmcFS/NyiG+1o8W+?=
 =?iso-8859-1?Q?buBpBC6eXMU7cd2grht4dAwNj770eRnsSgkAaraCueA7DFLO+t21oaXcFB?=
 =?iso-8859-1?Q?PZztNJAl2Z/ydiX7PgVi/zm7ZaZZtVIkJvOT+TcXsYb9NHkiFTaTVm8Vjy?=
 =?iso-8859-1?Q?EWAl5ecAoEpCy2eQTQC+d06mhaHILjqPCD?=
X-OriginatorOrg: silabs.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 42bb4061-1795-49f1-9bde-08dbf4e92e2b
X-MS-Exchange-CrossTenant-AuthSource: IA1PR11MB7773.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Dec 2023 16:50:54.3432
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 54dbd822-5231-4b20-944d-6f4abcd541fb
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 5x2OOApXsLOGf4S4KvWv8fHrQka6cga1K+q92WO2GB85qglifkYPe5ZqIkid/9bLxtI1XdaIQNAUeJ8Ffmr1PA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6454

Hello Dmitry,

On Monday 4 December 2023 16:55:37 CET Dmitry Antipov wrote:
>=20
> 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.
>=20
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/wireless/silabs/wfx/sta.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>=20
> 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;
>  }
>=20
> -static void wfx_set_mfp_ap(struct wfx_vif *wvif)
> +static int wfx_set_mfp_ap(struct wfx_vif *wvif)
>  {
>         struct ieee80211_vif *vif =3D wvif_to_vif(wvif);
>         struct sk_buff *skb =3D ieee80211_beacon_get(wvif->wdev->hw, vif,=
 0);
>         const int ieoffset =3D offsetof(struct ieee80211_mgmt, u.beacon.v=
ariable);
> -       const u16 *ptr =3D (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->dat=
a + ieoffset,
> -                                                skb->len - ieoffset);
>         const int pairwise_cipher_suite_count_offset =3D 8 / sizeof(u16);
>         const int pairwise_cipher_suite_size =3D 4 / sizeof(u16);
>         const int akm_suite_size =3D 4 / sizeof(u16);
> +       const u16 *ptr;
>=20
> +       if (unlikely(!skb))
> +               return -ENOMEM;
> +
> +       ptr =3D (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffse=
t,
> +                                     skb->len - ieoffset);
>         if (ptr) {

The code would be slightly better if we would invert this condition:

    if (!ptr)
            return -EINVAL;
=20
>                 ptr +=3D pairwise_cipher_suite_count_offset;
>                 if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
> -                       return;
> +                       return -EINVAL;
>                 ptr +=3D 1 + pairwise_cipher_suite_size * *ptr;
>                 if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
> -                       return;
> +                       return -EINVAL;
>                 ptr +=3D 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;
>  }
>=20
>  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 iee=
e80211_vif *vif,
>         wvif =3D (struct wfx_vif *)vif->drv_priv;
>         wfx_upload_ap_templates(wvif);
>         ret =3D 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);

>  }
>=20
>  void wfx_stop_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> --
> 2.43.0
>=20
>=20

I agree with the patch. Could you fix the cosmetics issues? I will take car=
e
of testing it on real hardware.


--=20
J=E9r=F4me Pouiller