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

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jerome Pouiller <Jerome.Pouiller@silabs.com>
Cc: devel@driverdev.osuosl.org, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"David S . Miller" <davem@davemloft.net>,
	Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH 01/12] staging: wfx: fix BA when device is AP and MFP is enabled
Date: Mon, 24 Aug 2020 12:50:42 +0300	[thread overview]
Message-ID: <20200824095042.GZ1793@kadam> (raw)
In-Reply-To: <20200820155858.351292-1-Jerome.Pouiller@silabs.com>

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