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

[Jérôme Pouiller]

On Monday 24 August 2020 11:50:42 CEST Dan Carpenter wrote:
> On Thu, Aug 20, 2020 at 05:58:47PM +0200, Jerome Pouiller wrote:
> > From: Jérôme Pouiller <jerome.pouiller@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> >  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?

I think the beacon come from hostapd (or any userspace application with
the necessary permissions). Indeed, it could be corrupted.

I have noticed that WLAN_EID_RSN is parsed at multiple places in the
kernel and I haven't seen any particular check :( (and WLAN_EID_RSN is
probably not the only dangerous IE).

Anyway, I am going to add a few checks on values of ptr.

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

-- 
Jérôme Pouiller