| Message ID | 20210617171522.3410951-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mwifiex: Avoid memset() over-write of WEP key_material | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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>
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
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,
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(-)