[2/2] mwifiex: Abort at too short BSS descriptor element
diff mbox series

Message ID 20190529125220.17066-3-tiwai@suse.de
State Accepted
Commit 685c9b7750bfacd6fc1db50d86579980593b7869
Delegated to: Kalle Valo
Headers show
Series
  • Buffer overflow / read checks in mwifiex
Related show

Commit Message

Takashi Iwai May 29, 2019, 12:52 p.m. UTC
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(+)

Comments

Brian Norris June 13, 2019, 5:49 p.m. UTC | #1
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
>
Takashi Iwai June 13, 2019, 6:12 p.m. UTC | #2
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
Brian Norris June 13, 2019, 6:38 p.m. UTC | #3
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
Brian Norris June 13, 2019, 8:26 p.m. UTC | #4
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
Brian Norris June 15, 2019, 12:19 a.m. UTC | #5
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

Patch
diff mbox series

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;