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 <[email protected]>
---
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) {
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,
--
2.43.0
Dmitry Antipov <[email protected]> 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 <[email protected]>
Patches like this should be tested on a real device.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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 <[email protected]>
> ---
> 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
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 <[email protected]>
---
v2: adjust branches according to maintainer's suggestions
---
drivers/net/wireless/silabs/wfx/sta.c | 42 ++++++++++++++++-----------
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
index 1b6c158457b4..537caf9d914a 100644
--- a/drivers/net/wireless/silabs/wfx/sta.c
+++ b/drivers/net/wireless/silabs/wfx/sta.c
@@ -336,29 +336,38 @@ 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 (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));
- }
+ if (unlikely(!skb))
+ return -ENOMEM;
+
+ ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
+ skb->len - ieoffset);
+ if (unlikely(!ptr))
+ return -EINVAL;
+
+ ptr += pairwise_cipher_suite_count_offset;
+ if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
+ return -EINVAL;
+
+ ptr += 1 + pairwise_cipher_suite_size * *ptr;
+ if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
+ return -EINVAL;
+
+ ptr += 1 + akm_suite_size * *ptr;
+ if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
+ return -EINVAL;
+
+ wfx_hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6));
+ return 0;
}
int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
@@ -376,8 +385,7 @@ int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
ret = wfx_hif_start(wvif, &vif->bss_conf, wvif->channel);
if (ret > 0)
return -EIO;
- wfx_set_mfp_ap(wvif);
- return ret;
+ return wfx_set_mfp_ap(wvif);
}
void wfx_stop_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
--
2.43.0
On Monday 4 December 2023 18:11:28 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 <[email protected]>
> ---
> v2: adjust branches according to maintainer's suggestions
> ---
> drivers/net/wireless/silabs/wfx/sta.c | 42 ++++++++++++++++-----------
> 1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/wireless/silabs/wfx/sta.c b/drivers/net/wireless/silabs/wfx/sta.c
> index 1b6c158457b4..537caf9d914a 100644
> --- a/drivers/net/wireless/silabs/wfx/sta.c
> +++ b/drivers/net/wireless/silabs/wfx/sta.c
> @@ -336,29 +336,38 @@ 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 (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));
> - }
> + if (unlikely(!skb))
> + return -ENOMEM;
> +
> + ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN, skb->data + ieoffset,
> + skb->len - ieoffset);
> + if (unlikely(!ptr))
> + return -EINVAL;
> +
> + ptr += pairwise_cipher_suite_count_offset;
> + if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
> + return -EINVAL;
> +
> + ptr += 1 + pairwise_cipher_suite_size * *ptr;
> + if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
> + return -EINVAL;
> +
> + ptr += 1 + akm_suite_size * *ptr;
> + if (WARN_ON(ptr > (u16 *)skb_tail_pointer(skb)))
> + return -EINVAL;
> +
> + wfx_hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6));
> + return 0;
> }
>
> int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> @@ -376,8 +385,7 @@ int wfx_start_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> ret = wfx_hif_start(wvif, &vif->bss_conf, wvif->channel);
> if (ret > 0)
> return -EIO;
> - wfx_set_mfp_ap(wvif);
> - return ret;
> + return wfx_set_mfp_ap(wvif);
> }
>
> void wfx_stop_ap(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> --
> 2.43.0
>
>
Tested-by: J?r?me Pouiller <[email protected]>
Acked-by: J?r?me Pouiller <[email protected]>
For the record, I tested with hostapd and this configuration:
channel=1
ctrl_interface=/var/run/hostapd
driver=nl80211
ht_capab=[SHORT-GI-20]
hw_mode=g
ieee80211n=1
ieee80211w=2
interface=wlan0
rsn_pairwise=CCMP
ssid=rpi-jpo-slv1
wpa=2
wpa_key_mgmt=WPA-PSK
wpa_passphrase=password
--
J?r?me Pouiller
Dmitry Antipov <[email protected]> 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 <[email protected]>
> Tested-by: Jérôme Pouiller <[email protected]>
> Acked-by: Jérôme Pouiller <[email protected]>
Patch applied to wireless-next.git, thanks.
fe0a7776d4d1 wifi: wfx: fix possible NULL pointer dereference in wfx_set_mfp_ap()
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches