* [PATCH v2] wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set
@ 2024-02-06 18:39 Kees Cook
2024-02-06 20:32 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2024-02-06 18:39 UTC (permalink / raw)
To: Brian Norris
Cc: Kees Cook, Kalle Valo, Dmitry Antipov, Johannes Berg, zuoqilin,
Ruan Jinjie, Thomas Gleixner, Christophe JAILLET,
Gustavo A . R . Silva, linux-wireless, Dan Carpenter,
Rafael Beims, David Lin, Lukas Wunner, Simon Horman, linux-kernel,
linux-hardening
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.
Link: https://github.com/KSPP/linux/issues/51 [1]
Cc: Brian Norris <briannorris@chromium.org>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Dmitry Antipov <dmantipov@yandex.ru>
Cc: Johannes Berg <johannes.berg@intel.com>
Cc: zuoqilin <zuoqilin@yulong.com>
Cc: Ruan Jinjie <ruanjinjie@huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Cc: linux-wireless@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
index 90e401100898..9ed90da4dfcf 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n.c
@@ -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 =
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
index 62f3c9a52a1d..3adc447b715f 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -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 {
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
index a2ddac363b10..0326b121747c 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -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)
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set
2024-02-06 18:39 [PATCH v2] wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set Kees Cook
@ 2024-02-06 20:32 ` Gustavo A. R. Silva
2024-02-07 9:56 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2024-02-06 20:32 UTC (permalink / raw)
To: Kees Cook, Brian Norris
Cc: Kalle Valo, Dmitry Antipov, Johannes Berg, zuoqilin, Ruan Jinjie,
Thomas Gleixner, Christophe JAILLET, Gustavo A . R . Silva,
linux-wireless, Dan Carpenter, Rafael Beims, David Lin,
Lukas Wunner, Simon Horman, linux-kernel, linux-hardening
On 2/6/24 12:39, Kees Cook wrote:
> 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.
>
> Link: https://github.com/KSPP/linux/issues/51 [1]
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: Dmitry Antipov <dmantipov@yandex.ru>
> Cc: Johannes Berg <johannes.berg@intel.com>
> Cc: zuoqilin <zuoqilin@yulong.com>
> Cc: Ruan Jinjie <ruanjinjie@huawei.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> Cc: linux-wireless@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
> index 90e401100898..9ed90da4dfcf 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n.c
> @@ -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 =
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
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 62f3c9a52a1d..3adc447b715f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -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 {
> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
> index a2ddac363b10..0326b121747c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -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)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set
2024-02-06 20:32 ` Gustavo A. R. Silva
@ 2024-02-07 9:56 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2024-02-07 9:56 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Brian Norris, Kalle Valo, Dmitry Antipov, Johannes Berg, zuoqilin,
Ruan Jinjie, Thomas Gleixner, Christophe JAILLET,
Gustavo A . R . Silva, linux-wireless, Dan Carpenter,
Rafael Beims, David Lin, Lukas Wunner, Simon Horman, linux-kernel,
linux-hardening
On Tue, Feb 06, 2024 at 02:32:32PM -0600, Gustavo A. R. Silva wrote:
>
>
> On 2/6/24 12:39, Kees Cook wrote:
> > 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.
> >
> > Link: https://github.com/KSPP/linux/issues/51 [1]
> > Cc: Brian Norris <briannorris@chromium.org>
> > Cc: Kalle Valo <kvalo@kernel.org>
> > Cc: Dmitry Antipov <dmantipov@yandex.ru>
> > Cc: Johannes Berg <johannes.berg@intel.com>
> > Cc: zuoqilin <zuoqilin@yulong.com>
> > Cc: Ruan Jinjie <ruanjinjie@huawei.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> > Cc: linux-wireless@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
> > index 90e401100898..9ed90da4dfcf 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/11n.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/11n.c
> > @@ -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 =
> 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);
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.
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-02-07 9:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-06 18:39 [PATCH v2] wifi: mwifiex: Refactor 1-element array into flexible array in struct mwifiex_ie_types_chan_list_param_set Kees Cook
2024-02-06 20:32 ` Gustavo A. R. Silva
2024-02-07 9:56 ` Kees Cook
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).