* [PATCH] mwifiex: Avoid memset() over-write of WEP key_material
@ 2021-06-17 17:15 Kees Cook
2021-06-17 19:29 ` Brian Norris
2021-06-22 15:25 ` Kalle Valo
0 siblings, 2 replies; 3+ messages in thread
From: Kees Cook @ 2021-06-17 17:15 UTC (permalink / raw)
To: Kalle Valo
Cc: Kees Cook, Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
Xinming Hu, David S. Miller, Jakub Kicinski, linux-kernel,
linux-wireless, netdev, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring array fields.
When preparing to call mwifiex_set_keyparamset_wep(), key_material is
treated very differently from its structure layout (which has only a
single struct mwifiex_ie_type_key_param_set). Instead, add a new type to
the union so memset() can correctly reason about the size of the
structure.
Note that the union ("params", 196 bytes) containing key_material was
not large enough to hold the target of this memset(): sizeof(struct
mwifiex_ie_type_key_param_set) == 60, NUM_WEP_KEYS = 4, so 240
bytes, or 44 bytes past the end of "params". The good news is that
it appears that the command buffer, as allocated, is 2048 bytes
(MWIFIEX_SIZE_OF_CMD_BUFFER), so no neighboring memory appears to be
getting clobbered.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/net/wireless/marvell/mwifiex/fw.h | 6 ++++++
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 11 ++++++-----
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
index 470d669c7f14..2ff23ab259ab 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -995,6 +995,11 @@ struct host_cmd_ds_802_11_key_material {
struct mwifiex_ie_type_key_param_set key_param_set;
} __packed;
+struct host_cmd_ds_802_11_key_material_wep {
+ __le16 action;
+ struct mwifiex_ie_type_key_param_set key_param_set[NUM_WEP_KEYS];
+} __packed;
+
struct host_cmd_ds_gen {
__le16 command;
__le16 size;
@@ -2347,6 +2352,7 @@ struct host_cmd_ds_command {
struct host_cmd_ds_wmm_get_status get_wmm_status;
struct host_cmd_ds_802_11_key_material key_material;
struct host_cmd_ds_802_11_key_material_v2 key_material_v2;
+ struct host_cmd_ds_802_11_key_material_wep key_material_wep;
struct host_cmd_ds_version_ext verext;
struct host_cmd_ds_mgmt_frame_reg reg_mask;
struct host_cmd_ds_remain_on_chan roc_cfg;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index d3a968ef21ef..48ea00da1fc9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -840,14 +840,15 @@ mwifiex_cmd_802_11_key_material_v1(struct mwifiex_private *priv,
}
if (!enc_key) {
- memset(&key_material->key_param_set, 0,
- (NUM_WEP_KEYS *
- sizeof(struct mwifiex_ie_type_key_param_set)));
+ struct host_cmd_ds_802_11_key_material_wep *key_material_wep =
+ (struct host_cmd_ds_802_11_key_material_wep *)key_material;
+ memset(key_material_wep->key_param_set, 0,
+ sizeof(key_material_wep->key_param_set));
ret = mwifiex_set_keyparamset_wep(priv,
- &key_material->key_param_set,
+ &key_material_wep->key_param_set[0],
&key_param_len);
cmd->size = cpu_to_le16(key_param_len +
- sizeof(key_material->action) + S_DS_GEN);
+ sizeof(key_material_wep->action) + S_DS_GEN);
return ret;
} else
memset(&key_material->key_param_set, 0,
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mwifiex: Avoid memset() over-write of WEP key_material
2021-06-17 17:15 [PATCH] mwifiex: Avoid memset() over-write of WEP key_material Kees Cook
@ 2021-06-17 19:29 ` Brian Norris
2021-06-22 15:25 ` Kalle Valo
1 sibling, 0 replies; 3+ messages in thread
From: Brian Norris @ 2021-06-17 19:29 UTC (permalink / raw)
To: Kees Cook
Cc: Kalle Valo, Amitkumar Karwar, Ganapathi Bhat,
Sharvari Harisangam, Xinming Hu, David S. Miller, Jakub Kicinski,
Linux Kernel, linux-wireless, <netdev@vger.kernel.org>,
linux-hardening
On Thu, Jun 17, 2021 at 10:15 AM Kees Cook <keescook@chromium.org> wrote:
>
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring array fields.
>
> When preparing to call mwifiex_set_keyparamset_wep(), key_material is
> treated very differently from its structure layout (which has only a
> single struct mwifiex_ie_type_key_param_set). Instead, add a new type to
> the union so memset() can correctly reason about the size of the
> structure.
>
> Note that the union ("params", 196 bytes) containing key_material was
> not large enough to hold the target of this memset(): sizeof(struct
> mwifiex_ie_type_key_param_set) == 60, NUM_WEP_KEYS = 4, so 240
> bytes, or 44 bytes past the end of "params". The good news is that
> it appears that the command buffer, as allocated, is 2048 bytes
> (MWIFIEX_SIZE_OF_CMD_BUFFER), so no neighboring memory appears to be
> getting clobbered.
Yeah, this union vs. the underlying buffer size always throws me for a
loop on figuring out whether there's truly a buffer overflow on some
of this stuff...
> Signed-off-by: Kees Cook <keescook@chromium.org>
Looks like a valid refactor to me:
Reviewed-by: Brian Norris <briannorris@chromium.org>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mwifiex: Avoid memset() over-write of WEP key_material
2021-06-17 17:15 [PATCH] mwifiex: Avoid memset() over-write of WEP key_material Kees Cook
2021-06-17 19:29 ` Brian Norris
@ 2021-06-22 15:25 ` Kalle Valo
1 sibling, 0 replies; 3+ messages in thread
From: Kalle Valo @ 2021-06-22 15:25 UTC (permalink / raw)
To: Kees Cook
Cc: Kees Cook, Amitkumar Karwar, Ganapathi Bhat, Sharvari Harisangam,
Xinming Hu, David S. Miller, Jakub Kicinski, linux-kernel,
linux-wireless, netdev, linux-hardening
Kees Cook <keescook@chromium.org> wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring array fields.
>
> When preparing to call mwifiex_set_keyparamset_wep(), key_material is
> treated very differently from its structure layout (which has only a
> single struct mwifiex_ie_type_key_param_set). Instead, add a new type to
> the union so memset() can correctly reason about the size of the
> structure.
>
> Note that the union ("params", 196 bytes) containing key_material was
> not large enough to hold the target of this memset(): sizeof(struct
> mwifiex_ie_type_key_param_set) == 60, NUM_WEP_KEYS = 4, so 240
> bytes, or 44 bytes past the end of "params". The good news is that
> it appears that the command buffer, as allocated, is 2048 bytes
> (MWIFIEX_SIZE_OF_CMD_BUFFER), so no neighboring memory appears to be
> getting clobbered.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
Patch applied to wireless-drivers-next.git, thanks.
59c668d700be mwifiex: Avoid memset() over-write of WEP key_material
--
https://patchwork.kernel.org/project/linux-wireless/patch/20210617171522.3410951-1-keescook@chromium.org/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-22 15:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 17:15 [PATCH] mwifiex: Avoid memset() over-write of WEP key_material Kees Cook
2021-06-17 19:29 ` Brian Norris
2021-06-22 15:25 ` 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).