Re: [PATCH 01/12] staging: wfx: fix BA when device is AP and MFP is enabled

Dan Carpenter
2020-08-24T09:53:15Z

On Thu, Aug 20, 2020 at 05:58:47PM +0200, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@silabs.com>
>
> The protection of the management frames is mainly done by mac80211.
> However, frames for the management of the BlockAck sessions are directly
> sent by the device. These frames have to be protected if MFP is in use.
> So the driver has to pass the MFP configuration to the device.
>
> Until now, the BlockAck management frames were completely unprotected
> whatever the status of the MFP negotiation. So, some devices dropped
> these frames.
>
> The device has two knobs to control the MFP. One global and one per
> station. Normally, the driver should always enable global MFP. Then it
> should enable MFP on every station with which MFP was successfully
> negotiated. Unfortunately, the older firmwares only provide the
> global control.
>
> So, this patch enable global MFP as it is exposed in the beacon. Then it
> marks every station with which the MFP is effective.
>
> Thus, the support for the old firmwares is not so bad. It may only
> encounter some difficulties to negotiate BA sessions when the local
> device (the AP) is MFP capable (ieee80211w=1) but the station is not.
> The only solution for this case is to upgrade the firmware.
>
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
> ---
> drivers/staging/wfx/sta.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index ad63332f690c..9c1c8223a49f 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -434,7 +434,7 @@ int wfx_sta_add(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> wvif->link_id_map |= BIT(sta_priv->link_id);
> WARN_ON(!sta_priv->link_id);
> WARN_ON(sta_priv->link_id >= HIF_LINK_ID_MAX);
> - hif_map_link(wvif, sta->addr, 0, sta_priv->link_id);
> + hif_map_link(wvif, sta->addr, sta->mfp ? 2 : 0, sta_priv->link_id);
>
> return 0;
> }
> @@ -474,6 +474,25 @@ static int wfx_upload_ap_templates(struct wfx_vif *wvif)
> return 0;
> }
>
> +static void wfx_set_mfp_ap(struct wfx_vif *wvif)
> +{
> + struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
> + 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;
> + ptr += 1 + pairwise_cipher_suite_size * *ptr;

The value of "*ptr" comes from skb->data. How do we know that it
doesn't point to something beyond the end of the skb->data buffer?

> + ptr += 1 + akm_suite_size * *ptr;
> + hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6));
> + }
> +}

regards,
dan carpenter