* [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
@ 2020-12-08 12:45 Xiaohui Zhang
2020-12-08 14:29 ` Kalle Valo
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Xiaohui Zhang @ 2020-12-08 12:45 UTC (permalink / raw)
To: Xiaohui Zhang, Amitkumar Karwar, Ganapathi Bhat, Xinming Hu,
Kalle Valo, David S. Miller, Jakub Kicinski, linux-wireless,
netdev, linux-kernel
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
2020-12-08 12:45 [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan Xiaohui Zhang
@ 2020-12-08 14:29 ` Kalle Valo
2020-12-09 0:36 ` Julian Calaby
2021-01-09 15:08 ` Peter Seiderer
2 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2020-12-08 14:29 UTC (permalink / raw)
To: Xiaohui Zhang
Cc: Amitkumar Karwar, Ganapathi Bhat, Xinming Hu, David S. Miller,
Jakub Kicinski, linux-wireless, netdev, linux-kernel
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()?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
2020-12-08 12:45 [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan Xiaohui Zhang
2020-12-08 14:29 ` Kalle Valo
@ 2020-12-09 0:36 ` Julian Calaby
2021-01-09 15:08 ` Peter Seiderer
2 siblings, 0 replies; 8+ messages in thread
From: Julian Calaby @ 2020-12-09 0:36 UTC (permalink / raw)
To: Xiaohui Zhang
Cc: Amitkumar Karwar, Ganapathi Bhat, Xinming Hu, Kalle Valo,
David S. Miller, Jakub Kicinski, linux-wireless, netdev, LKML
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/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
2020-12-08 12:45 [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan Xiaohui Zhang
2020-12-08 14:29 ` Kalle Valo
2020-12-09 0:36 ` Julian Calaby
@ 2021-01-09 15:08 ` Peter Seiderer
2021-01-11 18:51 ` Brian Norris
2 siblings, 1 reply; 8+ messages in thread
From: Peter Seiderer @ 2021-01-09 15:08 UTC (permalink / raw)
To: Xiaohui Zhang
Cc: Amitkumar Karwar, Ganapathi Bhat, Xinming Hu, Kalle Valo,
David S. Miller, Jakub Kicinski, linux-wireless, netdev,
linux-kernel
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
2021-01-09 15:08 ` Peter Seiderer
@ 2021-01-11 18:51 ` Brian Norris
0 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2021-01-11 18:51 UTC (permalink / raw)
To: Peter Seiderer
Cc: Xiaohui Zhang, Amitkumar Karwar, Ganapathi Bhat, Xinming Hu,
Kalle Valo, David S. Miller, Jakub Kicinski, linux-wireless,
<netdev@vger.kernel.org>,
Linux Kernel
(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_zhangxiaohui@163.com/
On Sat, Jan 9, 2021 at 7:11 AM Peter Seiderer <ps.report@gmx.net> wrote:
> 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...
I pointed out this discrepancy already, taking a slightly different approach:
https://lore.kernel.org/linux-wireless/CA+ASDXPVu5S0Vm0aOcyqLN090u3BwA_nV358YwkpXuU223Ug9g@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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
2020-12-08 19:12 ` Brian Norris
@ 2020-12-17 16:28 ` Kalle Valo
0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2020-12-17 16:28 UTC (permalink / raw)
To: Brian Norris
Cc: Xiaohui Zhang, Amitkumar Karwar, Ganapathi Bhat, Xinming Hu,
David S. Miller, Jakub Kicinski, linux-wireless,
<netdev@vger.kernel.org>,
Linux Kernel
Brian Norris <briannorris@chromium.org> writes:
> On Tue, Dec 8, 2020 at 7:14 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().
>
> ^^ That's not really what you're doing any more, for the record. But
> then, describing "what" is not really the point of a commit message
> (that's what the code is for), so maybe that's not that important.
>
>> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
>> ---
>> drivers/net/wireless/marvell/mwifiex/scan.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
>> index c2a685f63..34293fd80 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
>> @@ -931,7 +931,7 @@ mwifiex_config_scan(struct mwifiex_private *priv,
>> wildcard_ssid_tlv->max_ssid_length = 0xfe;
>>
>> memcpy(wildcard_ssid_tlv->ssid,
>> - user_scan_in->ssid_list[i].ssid, ssid_len);
>> + user_scan_in->ssid_list[i].ssid, min_t(u32, ssid_len, 1));
>
> This *looks* like it should be wrong, because SSIDs are clearly longer
> than 1 byte in many cases, but you *are* right that this is what the
> struct is defined as:
>
> struct mwifiex_ie_types_wildcard_ssid_params {
> ...
> u8 ssid[1];
> };
>
> This feels like something that could use some confirmation from
> NXP/ex-Marvell folks if possible, but if not that, at least some
> creative testing. Did you actually test this patch, to make sure
> non-wildcard scans still work?
>
> Also, even if this is correct, it seems like it would be more correct
> to use 'sizeof(wildcard_ssid_tlv->ssid)' instead of a magic number 1.
Xiaohui, please respond to Brian's comments. If you ignore review
comments I have a hard time trusting your patches.
Also when you submit a new version you should mark it as v2. See more in
the wiki link below.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
2020-12-08 15:09 Xiaohui Zhang
@ 2020-12-08 19:12 ` Brian Norris
2020-12-17 16:28 ` Kalle Valo
0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2020-12-08 19:12 UTC (permalink / raw)
To: Xiaohui Zhang
Cc: Amitkumar Karwar, Ganapathi Bhat, Xinming Hu, Kalle Valo,
David S. Miller, Jakub Kicinski, linux-wireless,
<netdev@vger.kernel.org>,
Linux Kernel
On Tue, Dec 8, 2020 at 7:14 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().
^^ That's not really what you're doing any more, for the record. But
then, describing "what" is not really the point of a commit message
(that's what the code is for), so maybe that's not that important.
> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> ---
> drivers/net/wireless/marvell/mwifiex/scan.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
> index c2a685f63..34293fd80 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -931,7 +931,7 @@ mwifiex_config_scan(struct mwifiex_private *priv,
> wildcard_ssid_tlv->max_ssid_length = 0xfe;
>
> memcpy(wildcard_ssid_tlv->ssid,
> - user_scan_in->ssid_list[i].ssid, ssid_len);
> + user_scan_in->ssid_list[i].ssid, min_t(u32, ssid_len, 1));
This *looks* like it should be wrong, because SSIDs are clearly longer
than 1 byte in many cases, but you *are* right that this is what the
struct is defined as:
struct mwifiex_ie_types_wildcard_ssid_params {
...
u8 ssid[1];
};
This feels like something that could use some confirmation from
NXP/ex-Marvell folks if possible, but if not that, at least some
creative testing. Did you actually test this patch, to make sure
non-wildcard scans still work?
Also, even if this is correct, it seems like it would be more correct
to use 'sizeof(wildcard_ssid_tlv->ssid)' instead of a magic number 1.
Brian
>
> tlv_pos += (sizeof(wildcard_ssid_tlv->header)
> + le16_to_cpu(wildcard_ssid_tlv->header.len));
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan
@ 2020-12-08 15:09 Xiaohui Zhang
2020-12-08 19:12 ` Brian Norris
0 siblings, 1 reply; 8+ messages in thread
From: Xiaohui Zhang @ 2020-12-08 15:09 UTC (permalink / raw)
To: Xiaohui Zhang, Amitkumar Karwar, Ganapathi Bhat, Xinming Hu,
Kalle Valo, David S. Miller, Jakub Kicinski, linux-wireless,
netdev, linux-kernel
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, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
index c2a685f63..34293fd80 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -931,7 +931,7 @@ mwifiex_config_scan(struct mwifiex_private *priv,
wildcard_ssid_tlv->max_ssid_length = 0xfe;
memcpy(wildcard_ssid_tlv->ssid,
- user_scan_in->ssid_list[i].ssid, ssid_len);
+ user_scan_in->ssid_list[i].ssid, min_t(u32, ssid_len, 1));
tlv_pos += (sizeof(wildcard_ssid_tlv->header)
+ le16_to_cpu(wildcard_ssid_tlv->header.len));
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-11 18:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-08 12:45 [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_config_scan Xiaohui Zhang
2020-12-08 14:29 ` Kalle Valo
2020-12-09 0:36 ` Julian Calaby
2021-01-09 15:08 ` Peter Seiderer
2021-01-11 18:51 ` Brian Norris
2020-12-08 15:09 Xiaohui Zhang
2020-12-08 19:12 ` Brian Norris
2020-12-17 16:28 ` Kalle Valo
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).