wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set
PATCH v3 1/1 wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set
struct mwifiex_ie_types_chan_list_param_set::chan_scan_param is treated as a flexible array, so convert it into one so that it doesn't trip the array bounds sanitizer[1]. Only a few places were using sizeof() on the whole struct, so adjust those to follow the calculation pattern to avoid including the trailing single element.
Examining binary output differences doesn't appear to show any literal size values changing, though it is obfuscated a bit by the compiler adjusting register usage and stack spill slots, etc.
---
v3: catch two more cases of changed sizeof (gustavo)
v2: https://lore.kernel.org/linux-hardening/[email protected]/
v1: https://lore.kernel.org/linux-hardening/[email protected]/
--- drivers/net/wireless/marvell/mwifiex/11n.c | 12 +++++------- drivers/net/wireless/marvell/mwifiex/fw.h | 2 +- drivers/net/wireless/marvell/mwifiex/scan.c | 14 ++++++-------- 3 files changed, 12 insertions(+), 16 deletions(-)
drivers/net/wireless/marvell/mwifiex/11n.c +5 -7
@@ -392,12 +392,10 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, chan_list = (struct mwifiex_ie_types_chan_list_param_set *) *buffer; -memset (chan_list,0 , -sizeof (struct mwifiex_ie_types_chan_list_param_set )); +memset (chan_list,0 ,struct_size (chan_list, chan_scan_param,1 )); chan_list->header.type =cpu_to_le16 (TLV_TYPE_CHANLIST ); - chan_list->header.len =cpu_to_le16 ( -sizeof (struct mwifiex_ie_types_chan_list_param_set ) - -sizeof (struct mwifiex_ie_types_header )); + chan_list->header.len = +cpu_to_le16 (sizeof (struct mwifiex_chan_scan_param_set )); chan_list->chan_scan_param[0 ].chan_number = bss_desc->bcn_ht_oper->primary_chan; chan_list->chan_scan_param[0].radio_type =
@@ -411,8 +409,8 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, (bss_desc->bcn_ht_oper->ht_param & IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); - *buffer +=sizeof (struct mwifiex_ie_types_chan_list_param_set ); - ret_len +=sizeof (struct mwifiex_ie_types_chan_list_param_set ); + *buffer +=struct_size (chan_list, chan_scan_param,1 ); + ret_len +=struct_size (chan_list, chan_scan_param,1 ); }if (bss_desc->bcn_bss_co_2040) {
drivers/net/wireless/marvell/mwifiex/fw.h +1 -1
@@ -770,7 +770,7 @@ struct mwifiex_chan_scan_param_set {struct mwifiex_ie_types_chan_list_param_set {struct mwifiex_ie_types_header header; -struct mwifiex_chan_scan_param_set chan_scan_param[1 ]; +struct mwifiex_chan_scan_param_set chan_scan_param[]; } __packed;struct mwifiex_ie_types_rxba_sync {
drivers/net/wireless/marvell/mwifiex/scan.c +6 -8
@@ -664,15 +664,14 @@ mwifiex_scan_channel_list(struct mwifiex_private *priv,/* Copy the current channel TLV to the command being prepared */ -memcpy (chan_tlv_out->chan_scan_param + tlv_idx, +memcpy (&chan_tlv_out->chan_scan_param[tlv_idx], tmp_chan_list, -sizeof (chan_tlv_out->chan_scan_param)); +sizeof (*chan_tlv_out->chan_scan_param));/* Increment the TLV header length by the size appended */ le16_unaligned_add_cpu (&chan_tlv_out->header.len, -sizeof ( - chan_tlv_out->chan_scan_param)); +sizeof (*chan_tlv_out->chan_scan_param));/* * The tlv buffer length is set to the number of bytes
@@ -2369,12 +2368,11 @@ int mwifiex_cmd_802_11_bg_scan_config(struct mwifiex_private *priv, chan_idx <MWIFIEX_BG_SCAN_CHAN_MAX && bgscan_cfg_in->chan_list[chan_idx].chan_number; chan_idx++) { - temp_chan = chan_list_tlv->chan_scan_param + chan_idx; + temp_chan = &chan_list_tlv->chan_scan_param[chan_idx];/* Increment the TLV header length by size appended */ le16_unaligned_add_cpu (&chan_list_tlv->header.len, -sizeof ( - chan_list_tlv->chan_scan_param)); +sizeof (*chan_list_tlv->chan_scan_param)); temp_chan->chan_number = bgscan_cfg_in->chan_list[chan_idx].chan_number;
@@ -2413,7 +2411,7 @@ int mwifiex_cmd_802_11_bg_scan_config(struct mwifiex_private *priv, chan_scan_param);le16_unaligned_add_cpu (&chan_list_tlv->header.len, chan_num * -sizeof (chan_list_tlv->chan_scan_param[0 ])); +sizeof (*chan_list_tlv->chan_scan_param)); } tlv_pos += (sizeof(chan_list_tlv->header)
Signature · 1 line
2.34.1
Quoted 10 lines · Kees Cook
struct
mwifiex_ie_types_chan_list_param_set::chan_scan_paramis treated as a flexible array, so convert it into one so that it doesn't trip the array bounds sanitizer[1]. Only a few places were usingsizeof()on the whole struct, so adjust those to follow the calculation pattern to avoid including the trailing single element.Examining binary output differences doesn't appear to show any literal size values changing, though it is obfuscated a bit by the compiler adjusting register usage and stack spill slots, etc.
Quoted 12 lines
Cc Brian Norris <[email protected]>Cc Kalle Valo <[email protected]>Cc Dmitry Antipov <[email protected]>Cc Johannes Berg <[email protected]>Cc zuoqilin <[email protected]>Cc Ruan Jinjie <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Christophe JAILLET <[email protected]>Cc Gustavo A. R. Silva <[email protected]>Signed-off-by Kees Cook <[email protected]>
TLV and one-element arrays are always tricky to transform (into flex arrays), but (unless I'm missing something that the maintainers might point out) this looks good to me.
Thanks!
--
Gustavo
Quoted 101 lines
---
v3: catch two more cases of changed sizeof (gustavo)
v2: https://lore.kernel.org/linux-hardening/[email protected]/
v1: https://lore.kernel.org/linux-hardening/[email protected]/--- drivers/net/wireless/marvell/mwifiex/11n.c | 12 +++++------- drivers/net/wireless/marvell/mwifiex/fw.h | 2 +- drivers/net/wireless/marvell/mwifiex/scan.c | 14 ++++++-------- 3 files changed, 12 insertions(+), 16 deletions(-)drivers/net/wireless/marvell/mwifiex/11n.c +5
-7@@ -392,12 +392,10 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, chan_list = (struct mwifiex_ie_types_chan_list_param_set *) *buffer; -memset (chan_list,0 , -sizeof (struct mwifiex_ie_types_chan_list_param_set )); +memset (chan_list,0 ,struct_size (chan_list, chan_scan_param,1 )); chan_list->header.type =cpu_to_le16 (TLV_TYPE_CHANLIST ); - chan_list->header.len =cpu_to_le16 ( -sizeof (struct mwifiex_ie_types_chan_list_param_set ) - -sizeof (struct mwifiex_ie_types_header )); + chan_list->header.len = +cpu_to_le16 (sizeof (struct mwifiex_chan_scan_param_set )); chan_list->chan_scan_param[0 ].chan_number = bss_desc->bcn_ht_oper->primary_chan; chan_list->chan_scan_param[0].radio_type =@@ -411,8 +409,8 @@ mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, (bss_desc->bcn_ht_oper->ht_param & IEEE80211_HT_PARAM_CHA_SEC_OFFSET)); - *buffer +=sizeof (struct mwifiex_ie_types_chan_list_param_set ); - ret_len +=sizeof (struct mwifiex_ie_types_chan_list_param_set ); + *buffer +=struct_size (chan_list, chan_scan_param,1 ); + ret_len +=struct_size (chan_list, chan_scan_param,1 ); }if (bss_desc->bcn_bss_co_2040) {drivers/net/wireless/marvell/mwifiex/fw.h +1
-1@@ -770,7 +770,7 @@ struct mwifiex_chan_scan_param_set {struct mwifiex_ie_types_chan_list_param_set {struct mwifiex_ie_types_header header; -struct mwifiex_chan_scan_param_set chan_scan_param[1 ]; +struct mwifiex_chan_scan_param_set chan_scan_param[]; } __packed;struct mwifiex_ie_types_rxba_sync {drivers/net/wireless/marvell/mwifiex/scan.c +6
-8@@ -664,15 +664,14 @@ mwifiex_scan_channel_list(struct mwifiex_private *priv,/* Copy the current channel TLV to the command being prepared */ -memcpy (chan_tlv_out->chan_scan_param + tlv_idx, +memcpy (&chan_tlv_out->chan_scan_param[tlv_idx], tmp_chan_list, -sizeof (chan_tlv_out->chan_scan_param)); +sizeof (*chan_tlv_out->chan_scan_param));/* Increment the TLV header length by the size appended */ le16_unaligned_add_cpu (&chan_tlv_out->header.len, -sizeof ( - chan_tlv_out->chan_scan_param)); +sizeof (*chan_tlv_out->chan_scan_param));/* * The tlv buffer length is set to the number of bytes @@ -2369,12 +2368,11 @@ int mwifiex_cmd_802_11_bg_scan_config(struct mwifiex_private *priv, chan_idx <MWIFIEX_BG_SCAN_CHAN_MAX && bgscan_cfg_in->chan_list[chan_idx].chan_number; chan_idx++) { - temp_chan = chan_list_tlv->chan_scan_param + chan_idx; + temp_chan = &chan_list_tlv->chan_scan_param[chan_idx];/* Increment the TLV header length by size appended */ le16_unaligned_add_cpu (&chan_list_tlv->header.len, -sizeof ( - chan_list_tlv->chan_scan_param)); +sizeof (*chan_list_tlv->chan_scan_param)); temp_chan->chan_number = bgscan_cfg_in->chan_list[chan_idx].chan_number;@@ -2413,7 +2411,7 @@ int mwifiex_cmd_802_11_bg_scan_config(struct mwifiex_private *priv, chan_scan_param);le16_unaligned_add_cpu (&chan_list_tlv->header.len, chan_num * -sizeof (chan_list_tlv->chan_scan_param[0 ])); +sizeof (*chan_list_tlv->chan_scan_param)); } tlv_pos += (sizeof(chan_list_tlv->header)
Quoted 10 lines · Kees Cook
struct
mwifiex_ie_types_chan_list_param_set::chan_scan_paramis treated as a flexible array, so convert it into one so that it doesn't trip the array bounds sanitizer[1]. Only a few places were usingsizeof()on the whole struct, so adjust those to follow the calculation pattern to avoid including the trailing single element.Examining binary output differences doesn't appear to show any literal size values changing, though it is obfuscated a bit by the compiler adjusting register usage and stack spill slots, etc.
Quoted 13 lines
Cc Brian Norris <[email protected]>Cc Kalle Valo <[email protected]>Cc Dmitry Antipov <[email protected]>Cc Johannes Berg <[email protected]>Cc zuoqilin <[email protected]>Cc Ruan Jinjie <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Christophe JAILLET <[email protected]>Cc Gustavo A. R. Silva <[email protected]>Signed-off-by Kees Cook <[email protected]>Reviewed-by Gustavo A. R. Silva <[email protected]>
Patch applied to wireless-next.git, thanks.
14ddc470ba22 wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set
Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches