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

[Brian Norris <briannorris@chromium.org>]

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.

> +				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).

Brian

> +				return -EINVAL;
> +
>  			vendor_ie = (struct ieee_types_vendor_specific *)
>  					current_ptr;
>  
> -- 
> 2.16.4
>