wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set
PATCH v2 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 once place was using sizeof() on the whole struct (in 11n.c), so adjust it to follow the calculation pattern used by scan.c to avoid including the trailing single element.
--- drivers/net/wireless/marvell/mwifiex/11n.c | 8 +++----- drivers/net/wireless/marvell/mwifiex/fw.h | 2 +- drivers/net/wireless/marvell/mwifiex/scan.c | 14 ++++++-------- 3 files changed, 10 insertions(+), 14 deletions(-)
drivers/net/wireless/marvell/mwifiex/11n.c +3 -5
@@ -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 =
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 34 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 once place was usingsizeof()on the whole struct (in11n.c), so adjust it to follow the calculation pattern used byscan.cto avoid including the trailing single element.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]>--- drivers/net/wireless/marvell/mwifiex/11n.c | 8 +++----- drivers/net/wireless/marvell/mwifiex/fw.h | 2 +- drivers/net/wireless/marvell/mwifiex/scan.c | 14 ++++++-------- 3 files changed, 10 insertions(+), 14 deletions(-)drivers/net/wireless/marvell/mwifiex/11n.c +0
-2@@ -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 ));
drivers/net/wireless/marvell/mwifiex/11n.c mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, · line 401
Quoted 10 lines
drivers/net/wireless/marvell/mwifiex/11n.c +3
-3+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 =
This probably still needs a bit more work.
There are a couple more instances of sizeof() that should probably be audited and adjusted:
drivers/net/wireless/marvell/mwifiex/11n.c:414: *buffer += sizeof(struct mwifiex_ie_types_chan_list_param_set);drivers/net/wireless/marvell/mwifiex/11n.c:415: ret_len += sizeof(struct mwifiex_ie_types_chan_list_param_set);
--
Gustavo
Quoted 60 lines
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 48 lines · Gustavo A. R. Silva
Quoted 33 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 once place was usingsizeof()on the whole struct (in11n.c), so adjust it to follow the calculation pattern used byscan.cto avoid including the trailing single element.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]>--- drivers/net/wireless/marvell/mwifiex/11n.c | 8 +++----- drivers/net/wireless/marvell/mwifiex/fw.h | 2 +- drivers/net/wireless/marvell/mwifiex/scan.c | 14 ++++++-------- 3 files changed, 10 insertions(+), 14 deletions(-)drivers/net/wireless/marvell/mwifiex/11n.c +0
-2@@ -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 ));drivers/net/wireless/marvell/mwifiex/11n.c mwifiex_cmd_append_11n_tlv(struct mwifiex_private *priv, · line 400
Quoted 10 lines
drivers/net/wireless/marvell/mwifiex/11n.c +3
-3+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 =This probably still needs a bit more work.
Quoted 5 lines
There are a couple more instances of
sizeof()that should probably be audited and adjusted:
drivers/net/wireless/marvell/mwifiex/11n.c:414: *buffer +=sizeof(struct mwifiex_ie_types_chan_list_param_set);drivers/net/wireless/marvell/mwifiex/11n.c:415:ret_len+=sizeof(struct mwifiex_ie_types_chan_list_param_set);
Good call. I think this is the right delta:
- *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);
I will send a v3.
Signature · 1 line
Kees Cook