[PATCH 0/2] Buffer overflow / read checks in mwifiex

[PATCH 0/2] Buffer overflow / read checks in mwifiex

[Takashi Iwai]

Hi,

this is fixes for a few spots that perform memcpy() without checking
the source and the destination size in mwifiex driver, which may lead
to buffer overflows or read over boundary.


Takashi

===

Takashi Iwai (2):
  mwifiex: Fix possible buffer overflows at parsing bss descriptor
  mwifiex: Abort at too short BSS descriptor element

 drivers/net/wireless/marvell/mwifiex/scan.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.16.4

[PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor

[Takashi Iwai]

mwifiex_update_bss_desc_with_ie() calls memcpy() unconditionally in
a couple places without checking the destination size.  Since the
source is given from user-space, this may trigger a heap buffer
overflow.

Fix it by putting the length check before performing memcpy().

This fix addresses CVE-2019-3846.

Reported-by: huangwen <huangwen@venustech.com.cn>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/net/wireless/marvell/mwifiex/scan.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
index 935778ec9a1b..64ab6fe78c0d 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1247,6 +1247,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
 		}
 		switch (element_id) {
 		case WLAN_EID_SSID:
+			if (element_len > IEEE80211_MAX_SSID_LEN)
+				return -EINVAL;
 			bss_entry->ssid.ssid_len = element_len;
 			memcpy(bss_entry->ssid.ssid, (current_ptr + 2),
 			       element_len);
@@ -1256,6 +1258,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
 			break;
 
 		case WLAN_EID_SUPP_RATES:
+			if (element_len > MWIFIEX_SUPPORTED_RATES)
+				return -EINVAL;
 			memcpy(bss_entry->data_rates, current_ptr + 2,
 			       element_len);
 			memcpy(bss_entry->supported_rates, current_ptr + 2,
-- 
2.16.4

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

[Takashi Iwai]

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))
+				return -EINVAL;
 			fh_param_set =
 				(struct ieee_types_fh_param_set *) current_ptr;
 			memcpy(&bss_entry->phy_param_set.fh_param_set,
@@ -1277,6 +1279,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
 			break;
 
 		case WLAN_EID_DS_PARAMS:
+			if (element_len + 2 < sizeof(*ds_param_set))
+				return -EINVAL;
 			ds_param_set =
 				(struct ieee_types_ds_param_set *) current_ptr;
 
@@ -1288,6 +1292,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
 			break;
 
 		case WLAN_EID_CF_PARAMS:
+			if (element_len + 2 < sizeof(*cf_param_set))
+				return -EINVAL;
 			cf_param_set =
 				(struct ieee_types_cf_param_set *) current_ptr;
 			memcpy(&bss_entry->ss_param_set.cf_param_set,
@@ -1296,6 +1302,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
 			break;
 
 		case WLAN_EID_IBSS_PARAMS:
+			if (element_len + 2 < sizeof(*ibss_param_set))
+				return -EINVAL;
 			ibss_param_set =
 				(struct ieee_types_ibss_param_set *)
 				current_ptr;
@@ -1305,10 +1313,14 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
 			break;
 
 		case WLAN_EID_ERP_INFO:
+			if (!element_len)
+				return -EINVAL;
 			bss_entry->erp_flags = *(current_ptr + 2);
 			break;
 
 		case WLAN_EID_PWR_CONSTRAINT:
+			if (!element_len)
+				return -EINVAL;
 			bss_entry->local_constraint = *(current_ptr + 2);
 			bss_entry->sensed_11h = true;
 			break;
@@ -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))
+				return -EINVAL;
+
 			vendor_ie = (struct ieee_types_vendor_specific *)
 					current_ptr;
 
-- 
2.16.4

Re: [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor

[Kalle Valo]

Takashi Iwai <tiwai@suse.de> wrote:

> mwifiex_update_bss_desc_with_ie() calls memcpy() unconditionally in
> a couple places without checking the destination size.  Since the
> source is given from user-space, this may trigger a heap buffer
> overflow.
> 
> Fix it by putting the length check before performing memcpy().
> 
> This fix addresses CVE-2019-3846.
> 
> Reported-by: huangwen <huangwen@venustech.com.cn>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

2 patches applied to wireless-drivers.git, thanks.

13ec7f10b87f mwifiex: Fix possible buffer overflows at parsing bss descriptor
685c9b7750bf mwifiex: Abort at too short BSS descriptor element

-- 
https://patchwork.kernel.org/patch/10967049/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Brian Norris]

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
> 

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

[Takashi Iwai]

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

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

[Brian Norris]

On Thu, Jun 13, 2019 at 08:12:39PM +0200, Takashi Iwai wrote:
> On Thu, 13 Jun 2019 19:49:40 +0200,
> Brian Norris wrote:
> > On Wed, May 29, 2019 at 02:52:20PM +0200, Takashi Iwai wrote:
> > > --- 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.

I'll wait until we straighten out the other (potentially) real bug.
Don't want to make needless conflicts.

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

I think this is the right place for one check (the memcmp() is right
after this line anyway) -- the minimum length just should be smaller.

e.g.:

sizeof(struct ieee80211_vendor_ie) // this is still technically 1 byte too large I think
or
offsetof(struct ieee80211_vendor_ie, oui_type) // not sure if this is the cleanest...

If it's smaller than that, we can still say -EINVAL.

Then, we can go with:

if (element_len < sizeof(wpa_oui)
	continue;

or similar.

So, I might say:

	/* Vendor IEs must at least contain the OUI. */
	if (total_ie_len < offsetof(struct ieee80211_vendor_ie, oui_type))
		return -EINVAL;

	/* If the IE still isn't long enough, it's not a match. */
	if (element_len < sizeof(wpa_oui))
		continue;

Brian

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

[Brian Norris]

On Thu, Jun 13, 2019 at 11:38 AM Brian Norris <briannorris@chromium.org> wrote:
> So, I might say:
>
>         /* Vendor IEs must at least contain the OUI. */
>         if (total_ie_len < offsetof(struct ieee80211_vendor_ie, oui_type))
>                 return -EINVAL;
>
>         /* If the IE still isn't long enough, it's not a match. */
>         if (element_len < sizeof(wpa_oui))
>                 continue;

That would of course need to be break, not continue, to properly skip
to the next IE.

Brian

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

[Brian Norris]

Hi,

On Thu, Jun 13, 2019 at 10:49 AM Brian Norris <briannorris@chromium.org> wrote:
> "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.
[...]
> 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).

So I just decided to send some patches myself, for both of my notes:

[PATCH 5.2 1/2] mwifiex: Don't abort on small, spec-compliant vendor IEs
https://patchwork.kernel.org/patch/10996895/

[PATCH 2/2] mwifiex: use 'total_ie_len' in mwifiex_update_bss_desc_with_ie()
https://patchwork.kernel.org/patch/10996893/

I'd appreciate your review.

Regards,
Brian