| Message ID | 20201208154343.6946-1-ruc_zhangxiaohui@163.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] mwifiex: Fix possible buffer overflows in mwifiex_uap_bss_param_prepare | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
(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 >
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) +