* [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare
@ 2020-12-08 11:36 Xiaohui Zhang
2020-12-08 14:28 ` Kalle Valo
0 siblings, 1 reply; 5+ messages in thread
From: Xiaohui Zhang @ 2020-12-08 11:36 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_uap_bss_param_prepare() 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/uap_cmd.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
index b48a85d79..fb937c7ee 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
@@ -496,13 +496,16 @@ mwifiex_uap_bss_param_prepare(u8 *tlv, void *cmd_buf, u16 *param_size)
struct mwifiex_ie_types_wmmcap *wmm_cap;
struct mwifiex_uap_bss_param *bss_cfg = cmd_buf;
int i;
+ int ssid_size;
u16 cmd_size = *param_size;
if (bss_cfg->ssid.ssid_len) {
ssid = (struct host_cmd_tlv_ssid *)tlv;
ssid->header.type = cpu_to_le16(TLV_TYPE_UAP_SSID);
ssid->header.len = cpu_to_le16((u16)bss_cfg->ssid.ssid_len);
- memcpy(ssid->ssid, bss_cfg->ssid.ssid, bss_cfg->ssid.ssid_len);
+ ssid_size = bss_cfg->ssid.ssid_len > strlen(ssid->ssid) ?
+ strlen(ssid->ssid) : bss_cfg->ssid.ssid_len;
+ memcpy(ssid->ssid, bss_cfg->ssid.ssid, ssid_size);
cmd_size += sizeof(struct mwifiex_ie_types_header) +
bss_cfg->ssid.ssid_len;
tlv += sizeof(struct mwifiex_ie_types_header) +
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare
2020-12-08 11:36 [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare Xiaohui Zhang
@ 2020-12-08 14:28 ` Kalle Valo
0 siblings, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2020-12-08 14:28 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_uap_bss_param_prepare() 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/uap_cmd.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> index b48a85d79..fb937c7ee 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> @@ -496,13 +496,16 @@ mwifiex_uap_bss_param_prepare(u8 *tlv, void *cmd_buf, u16 *param_size)
> struct mwifiex_ie_types_wmmcap *wmm_cap;
> struct mwifiex_uap_bss_param *bss_cfg = cmd_buf;
> int i;
> + int ssid_size;
> u16 cmd_size = *param_size;
>
> if (bss_cfg->ssid.ssid_len) {
> ssid = (struct host_cmd_tlv_ssid *)tlv;
> ssid->header.type = cpu_to_le16(TLV_TYPE_UAP_SSID);
> ssid->header.len = cpu_to_le16((u16)bss_cfg->ssid.ssid_len);
> - memcpy(ssid->ssid, bss_cfg->ssid.ssid, bss_cfg->ssid.ssid_len);
> + ssid_size = bss_cfg->ssid.ssid_len > strlen(ssid->ssid) ?
> + strlen(ssid->ssid) : bss_cfg->ssid.ssid_len;
> + memcpy(ssid->ssid, bss_cfg->ssid.ssid, ssid_size);
I think using min_t() is cleaner. Then you would not need to add a
temporary variable.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare
2020-12-08 15:43 Xiaohui Zhang
2020-12-08 16:05 ` Kalle Valo
@ 2020-12-08 18:56 ` Brian Norris
1 sibling, 0 replies; 5+ messages in thread
From: Brian Norris @ 2020-12-08 18:56 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
(FWIW, this author's mail has been routed to my spam mailbox. That's
partly my fault and/or my "choice" of mail provider, but that's why I
only see these once Kalle replies to them.)
On Tue, Dec 8, 2020 at 8:03 AM Xiaohui Zhang <ruc_zhangxiaohui@163.com> wrote:
>
> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
>
> mwifiex_uap_bss_param_prepare() 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/uap_cmd.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> index b48a85d79..937c75e89 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> @@ -502,7 +502,8 @@ mwifiex_uap_bss_param_prepare(u8 *tlv, void *cmd_buf, u16 *param_size)
> ssid = (struct host_cmd_tlv_ssid *)tlv;
> ssid->header.type = cpu_to_le16(TLV_TYPE_UAP_SSID);
> ssid->header.len = cpu_to_le16((u16)bss_cfg->ssid.ssid_len);
> - memcpy(ssid->ssid, bss_cfg->ssid.ssid, bss_cfg->ssid.ssid_len);
> + memcpy(ssid->ssid, bss_cfg->ssid.ssid,
> + min_t(u32, bss_cfg->ssid.ssid_len, strlen(ssid->ssid)));
This strlen() check makes no sense to me. We are *writing* to
ssid->ssid, so its initial contents are either zero or garbage --
strlen() will either give a zero or unpredictable value. I'm pretty
sure that's not what you intend.
On the other hand, it's hard to determine what the proper bound here
*should* be. This 'ssid' struct is really just a pointer into
mwifiex_cmd_uap_sys_config()'s uap_sys_config (struct
host_cmd_ds_sys_config), which doesn't have any defined length -- its
length is only given by way of its surrounding buffers/structs.
Altogether, the code is hard to reason about.
Anyway, this patch is wrong, so NAK.
Brian
> cmd_size += sizeof(struct mwifiex_ie_types_header) +
> bss_cfg->ssid.ssid_len;
> tlv += sizeof(struct mwifiex_ie_types_header) +
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare
2020-12-08 15:43 Xiaohui Zhang
@ 2020-12-08 16:05 ` Kalle Valo
2020-12-08 18:56 ` Brian Norris
1 sibling, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2020-12-08 16:05 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_uap_bss_param_prepare() 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>
When you submit a new version mark it as v2:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#patch_version_missing
But this is just for the future, no need to resend because of this.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare
@ 2020-12-08 15:43 Xiaohui Zhang
2020-12-08 16:05 ` Kalle Valo
2020-12-08 18:56 ` Brian Norris
0 siblings, 2 replies; 5+ messages in thread
From: Xiaohui Zhang @ 2020-12-08 15:43 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_uap_bss_param_prepare() 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/uap_cmd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
index b48a85d79..937c75e89 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
@@ -502,7 +502,8 @@ mwifiex_uap_bss_param_prepare(u8 *tlv, void *cmd_buf, u16 *param_size)
ssid = (struct host_cmd_tlv_ssid *)tlv;
ssid->header.type = cpu_to_le16(TLV_TYPE_UAP_SSID);
ssid->header.len = cpu_to_le16((u16)bss_cfg->ssid.ssid_len);
- memcpy(ssid->ssid, bss_cfg->ssid.ssid, bss_cfg->ssid.ssid_len);
+ memcpy(ssid->ssid, bss_cfg->ssid.ssid,
+ min_t(u32, bss_cfg->ssid.ssid_len, strlen(ssid->ssid)));
cmd_size += sizeof(struct mwifiex_ie_types_header) +
bss_cfg->ssid.ssid_len;
tlv += sizeof(struct mwifiex_ie_types_header) +
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-08 21:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-08 11:36 [PATCH 1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare Xiaohui Zhang
2020-12-08 14:28 ` Kalle Valo
2020-12-08 15:43 Xiaohui Zhang
2020-12-08 16:05 ` Kalle Valo
2020-12-08 18:56 ` Brian Norris
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.