| Message ID | 20190529125220.17066-3-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 685c9b7750bfacd6fc1db50d86579980593b7869 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | Buffer overflow / read checks in mwifiex | expand |
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 >
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
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
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
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
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;
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(+)