(Note: this is version 1; there's a later version posted, which does not have a v2 tag...)
https://lore.kernel.org/linux-wireless/20201208150951.35866-1-ruc_zhangxiao...@163.com/ On Sat, Jan 9, 2021 at 7:11 AM Peter Seiderer <ps.rep...@gmx.net> wrote: > On Tue, 8 Dec 2020 20:45:23 +0800, Xiaohui Zhang <ruc_zhangxiao...@163.com> > wrote: > > From: Zhang Xiaohui <ruc_zhangxiao...@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_zhangxiao...@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... I pointed out this discrepancy already, taking a slightly different approach: https://lore.kernel.org/linux-wireless/ca+asdxpvu5s0vm0aocyqln090u3bwa_nv358ywkpxuu223u...@mail.gmail.com/ > 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)) Ah, good catch. So this may not be a true overflow issue at all, even if it's confusing and bad code. The "problem" is that this piece of the struct is variable-length, and unless we're going to dynamically resize/reallocate the whole buffer, we have to assume the initial allocation was large enough (and per your note, it is). > For sure something to improve here instead of using a confusing 'u8 ssid[1]' > in struct mwifiex_ie_types_wildcard_ssid_params... Yep. Brian
