* [PATCH 0/2] Buffer overflow / read checks in mwifiex @ 2019-05-29 12:52 Takashi Iwai 2019-05-29 12:52 ` [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor Takashi Iwai 2019-05-29 12:52 ` [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element Takashi Iwai 0 siblings, 2 replies; 9+ messages in thread From: Takashi Iwai @ 2019-05-29 12:52 UTC (permalink / raw) To: linux-wireless Cc: Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor 2019-05-29 12:52 [PATCH 0/2] Buffer overflow / read checks in mwifiex Takashi Iwai @ 2019-05-29 12:52 ` Takashi Iwai 2019-05-30 11:22 ` Kalle Valo 2019-05-29 12:52 ` [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element Takashi Iwai 1 sibling, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2019-05-29 12:52 UTC (permalink / raw) To: linux-wireless Cc: Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor 2019-05-29 12:52 ` [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor Takashi Iwai @ 2019-05-30 11:22 ` Kalle Valo 0 siblings, 0 replies; 9+ messages in thread From: Kalle Valo @ 2019-05-30 11:22 UTC (permalink / raw) To: Takashi Iwai Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element 2019-05-29 12:52 [PATCH 0/2] Buffer overflow / read checks in mwifiex Takashi Iwai 2019-05-29 12:52 ` [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor Takashi Iwai @ 2019-05-29 12:52 ` Takashi Iwai 2019-06-13 17:49 ` Brian Norris 1 sibling, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2019-05-29 12:52 UTC (permalink / raw) To: linux-wireless Cc: Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element 2019-05-29 12:52 ` [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element Takashi Iwai @ 2019-06-13 17:49 ` Brian Norris 2019-06-13 18:12 ` Takashi Iwai 2019-06-15 0:19 ` Brian Norris 0 siblings, 2 replies; 9+ messages in thread From: Brian Norris @ 2019-06-13 17:49 UTC (permalink / raw) To: Takashi Iwai Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element 2019-06-13 17:49 ` Brian Norris @ 2019-06-13 18:12 ` Takashi Iwai 2019-06-13 18:38 ` Brian Norris 2019-06-15 0:19 ` Brian Norris 1 sibling, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2019-06-13 18:12 UTC (permalink / raw) To: Brian Norris Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element 2019-06-13 18:12 ` Takashi Iwai @ 2019-06-13 18:38 ` Brian Norris 2019-06-13 20:26 ` Brian Norris 0 siblings, 1 reply; 9+ messages in thread From: Brian Norris @ 2019-06-13 18:38 UTC (permalink / raw) To: Takashi Iwai Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element 2019-06-13 18:38 ` Brian Norris @ 2019-06-13 20:26 ` Brian Norris 0 siblings, 0 replies; 9+ messages in thread From: Brian Norris @ 2019-06-13 20:26 UTC (permalink / raw) To: Takashi Iwai Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element 2019-06-13 17:49 ` Brian Norris 2019-06-13 18:12 ` Takashi Iwai @ 2019-06-15 0:19 ` Brian Norris 1 sibling, 0 replies; 9+ messages in thread From: Brian Norris @ 2019-06-15 0:19 UTC (permalink / raw) To: Takashi Iwai Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer, Marcus Meissner 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-15 0:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-29 12:52 [PATCH 0/2] Buffer overflow / read checks in mwifiex Takashi Iwai 2019-05-29 12:52 ` [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor Takashi Iwai 2019-05-30 11:22 ` Kalle Valo 2019-05-29 12:52 ` [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element Takashi Iwai 2019-06-13 17:49 ` Brian Norris 2019-06-13 18:12 ` Takashi Iwai 2019-06-13 18:38 ` Brian Norris 2019-06-13 20:26 ` Brian Norris 2019-06-15 0:19 ` Brian Norris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).