| Message ID | 20201208124523.8169-1-ruc_zhangxiaohui@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan | expand |
Xiaohui Zhang <ruc_zhangxiaohui@163.com> writes: > From: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > > mwifiex_config_scan() calls memcpy() without checking > the destination size may trigger a buffer overflower, > which a local user could use to cause denial of service > or the execution of arbitrary code. > Fix it by putting the length check before calling memcpy(). > > Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > --- > drivers/net/wireless/marvell/mwifiex/scan.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c > index c2a685f63..b1d90678f 100644 > --- a/drivers/net/wireless/marvell/mwifiex/scan.c > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c > @@ -930,6 +930,8 @@ mwifiex_config_scan(struct mwifiex_private *priv, > "DIRECT-", 7)) > wildcard_ssid_tlv->max_ssid_length = 0xfe; > > + if (ssid_len > 1) > + ssid_len = 1; > memcpy(wildcard_ssid_tlv->ssid, > user_scan_in->ssid_list[i].ssid, ssid_len); min_t()?
Hi Xiaohui, On Wed, Dec 9, 2020 at 12:07 AM Xiaohui Zhang <ruc_zhangxiaohui@163.com> wrote: > > From: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > > mwifiex_config_scan() calls memcpy() without checking > the destination size may trigger a buffer overflower, > which a local user could use to cause denial of service > or the execution of arbitrary code. > Fix it by putting the length check before calling memcpy(). > > Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > --- > drivers/net/wireless/marvell/mwifiex/scan.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c > index c2a685f63..b1d90678f 100644 > --- a/drivers/net/wireless/marvell/mwifiex/scan.c > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c > @@ -930,6 +930,8 @@ mwifiex_config_scan(struct mwifiex_private *priv, > "DIRECT-", 7)) > wildcard_ssid_tlv->max_ssid_length = 0xfe; > > + if (ssid_len > 1) > + ssid_len = 1; > memcpy(wildcard_ssid_tlv->ssid, > user_scan_in->ssid_list[i].ssid, ssid_len); Can ssid_len ever be 0 here? If it can't, should we just set ssid_len to 1 unconditionally? If it can, should we just skip the memcpy as it won't do anything? Thanks, -- Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/
Hello Zhang, On Tue, 8 Dec 2020 20:45:23 +0800, Xiaohui Zhang <ruc_zhangxiaohui@163.com> wrote: > From: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > > mwifiex_config_scan() calls memcpy() without checking > the destination size may trigger a buffer overflower, > which a local user could use to cause denial of service > or the execution of arbitrary code. > Fix it by putting the length check before calling memcpy(). > > Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > --- > drivers/net/wireless/marvell/mwifiex/scan.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c > index c2a685f63..b1d90678f 100644 > --- a/drivers/net/wireless/marvell/mwifiex/scan.c > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c > @@ -930,6 +930,8 @@ mwifiex_config_scan(struct mwifiex_private *priv, > "DIRECT-", 7)) > wildcard_ssid_tlv->max_ssid_length = 0xfe; > > + if (ssid_len > 1) > + ssid_len = 1; Why do your believe the available size is only '1'? A SSID is expected to be of size IEEE80211_MAX_SSID_LE/32 and the wildcard_ssid_tlv pointer is casted from tlv_pos (some lines above) which is a pointer/index into scan_cfg_out->tlv_buf... And the define (line 44) indicates there should be enough space for a SSID: 42 /* Memory needed to store a max number/size WildCard SSID TLV for a firmware 43 scan */ 44 #define WILDCARD_SSID_TLV_MAX_SIZE \ 45 (MWIFIEX_MAX_SSID_LIST_LENGTH * \ 46 (sizeof(struct mwifiex_ie_types_wildcard_ssid_params) \ 47 + IEEE80211_MAX_SSID_LEN)) For sure something to improve here instead of using a confusing 'u8 ssid[1]' in struct mwifiex_ie_types_wildcard_ssid_params... Regards, Peter > memcpy(wildcard_ssid_tlv->ssid, > user_scan_in->ssid_list[i].ssid, ssid_len); >
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index c2a685f63..b1d90678f 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -930,6 +930,8 @@ mwifiex_config_scan(struct mwifiex_private *priv, "DIRECT-", 7)) wildcard_ssid_tlv->max_ssid_length = 0xfe; + if (ssid_len > 1) + ssid_len = 1; memcpy(wildcard_ssid_tlv->ssid, user_scan_in->ssid_list[i].ssid, ssid_len);