Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element

[Takashi Iwai <tiwai@suse.de>]

On Thu, 13 Jun 2019 19:49:40 +0200,
Brian Norris wrote:
> 
> Hi Takashi,
> 
> On Wed, May 29, 2019 at 02:52:20PM +0200, Takashi Iwai wrote:
> > Currently mwifiex_update_bss_desc_with_ie() implicitly assumes that
> > the source descriptor entries contain the enough size for each type
> > and performs copying without checking the source size.  This may lead
> > to read over boundary.
> > 
> > Fix this by putting the source size check in appropriate places.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/scan.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
> > index 64ab6fe78c0d..c269a0de9413 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> > @@ -1269,6 +1269,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> >  			break;
> >  
> >  		case WLAN_EID_FH_PARAMS:
> > +			if (element_len + 2 < sizeof(*fh_param_set))
> 
> "element_len + 2" would be much more readable as "total_ie_len". (Same for
> several other usages in this patch.) I can send such a patch myself as a
> follow-up I suppose.

Yes, please.

> > +				return -EINVAL;
> >  			fh_param_set =
> >  				(struct ieee_types_fh_param_set *) current_ptr;
> >  			memcpy(&bss_entry->phy_param_set.fh_param_set,
> 
> [...]
> 
> > @@ -1349,6 +1361,9 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> >  			break;
> >  
> >  		case WLAN_EID_VENDOR_SPECIFIC:
> > +			if (element_len + 2 < sizeof(vendor_ie->vend_hdr))
> 
> Why 'sizeof(vendor_ie->vend_hdr)'? The (mwifiex-specific compare with the
> ieee80211.h generic struct ieee80211_vendor_ie) ieee_types_vendor_header struct
> includes the 'oui_subtype' and 'version' fields, which are not standard
> requirements for the vendor header (in fact, even the 4th byte of the
> OUI -- "oui_type" -- doesn't appear to be in the 802.11 specification).
> So it looks to me like you might be rejecting valid vendor headers (that
> we should just be skipping) that might have vendor-specific content with
> length 0 or 1 bytes.
> 
> It seems like we should only be validating the standard pieces (e.g., up to the
> length/OUI), and only after an appropriate OUI match, *then* validating the rest of
> the vendor element (the pieces we'll use later).

Hm, right, that looks too strict.  Instead we need to check right
before both memcmp()'s of OUI.


thanks,

Takashi