| Message ID | Y9xkECG3uTZ6T1dN@work (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7fcae8f7f8158d22e667ed55a40e6a1829cc55b0 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [next] wifi: mwifiex: Replace one-element arrays with flexible-array members | expand |
On Thu, Feb 02, 2023 at 07:32:00PM -0600, Gustavo A. R. Silva wrote: > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element arrays with flexible-array > members in multiple structures. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > This results in no differences in binary output. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/256 > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Reviewed-by: Kees Cook <keescook@chromium.org>
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote: > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element arrays with flexible-array > members in multiple structures. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > This results in no differences in binary output. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/256 > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Reviewed-by: Kees Cook <keescook@chromium.org> Patch applied to wireless-next.git, thanks. 7fcae8f7f815 wifi: mwifiex: Replace one-element arrays with flexible-array members
On Thu, Feb 02, 2023 at 07:32:00PM -0600, Gustavo A. R. Silva wrote: > One-element arrays are deprecated, and we are replacing them with flexible > array members instead. So, replace one-element arrays with flexible-array > members in multiple structures. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > This results in no differences in binary output. Sorry for blast from the past, but I have a question here. This change seems converts many of the flexible arrays in this driver. But what's behind this one? struct host_cmd_ds_802_11_scan_ext { u32 reserved; u8 tlv_buffer[1]; } __packed; AFAIU this needs also some care. On the real machine I have got this elo 16 17:51:58 surfacebook kernel: ------------[ cut here ]------------ elo 16 17:51:58 surfacebook kernel: memcpy: detected field-spanning write (size 243) of single field "ext_scan->tlv_buffer" at drivers/net/wireless/marvell/mwifiex/scan.c:2239 (size 1) elo 16 17:51:58 surfacebook kernel: WARNING: CPU: 0 PID: 498 at drivers/net/wireless/marvell/mwifiex/scan.c:2239 mwifiex_cmd_802_11_scan_ext+0x83/0x90 [mwifiex] which leads to memcpy(ext_scan->tlv_buffer, scan_cfg->tlv_buf, scan_cfg->tlv_buf_len); but the code allocates 2k or more for the command buffer, so this seems quite enough for 243 bytes.
On 21/08/24 14:26, Andy Shevchenko wrote: > On Thu, Feb 02, 2023 at 07:32:00PM -0600, Gustavo A. R. Silva wrote: >> One-element arrays are deprecated, and we are replacing them with flexible >> array members instead. So, replace one-element arrays with flexible-array >> members in multiple structures. >> >> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE >> routines on memcpy() and help us make progress towards globally >> enabling -fstrict-flex-arrays=3 [1]. >> >> This results in no differences in binary output. > > Sorry for blast from the past, but I have a question here. > > This change seems converts many of the flexible arrays in this driver. > But what's behind this one? > > struct host_cmd_ds_802_11_scan_ext { > u32 reserved; > u8 tlv_buffer[1]; > } __packed; > > > AFAIU this needs also some care. On the real machine I have got this > > elo 16 17:51:58 surfacebook kernel: ------------[ cut here ]------------ > elo 16 17:51:58 surfacebook kernel: memcpy: detected field-spanning write (size 243) of single field "ext_scan->tlv_buffer" at drivers/net/wireless/marvell/mwifiex/scan.c:2239 (size 1) > elo 16 17:51:58 surfacebook kernel: WARNING: CPU: 0 PID: 498 at drivers/net/wireless/marvell/mwifiex/scan.c:2239 mwifiex_cmd_802_11_scan_ext+0x83/0x90 [mwifiex] > > which leads to > > memcpy(ext_scan->tlv_buffer, scan_cfg->tlv_buf, scan_cfg->tlv_buf_len); > > but the code allocates 2k or more for the command buffer, so this seems > quite enough for 243 bytes. > I think this would do it: diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index e91def0afa14..d03129d5d24e 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -1627,7 +1627,7 @@ struct host_cmd_ds_802_11_scan_rsp { struct host_cmd_ds_802_11_scan_ext { u32 reserved; - u8 tlv_buffer[1]; + u8 tlv_buffer[]; } __packed; struct mwifiex_ie_types_bss_mode { diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c index e782d652cb93..f7153472e2a2 100644 --- a/drivers/net/wireless/marvell/mwifiex/scan.c +++ b/drivers/net/wireless/marvell/mwifiex/scan.c @@ -2536,8 +2536,7 @@ int mwifiex_ret_802_11_scan_ext(struct mwifiex_private *priv, ext_scan_resp = &resp->params.ext_scan; tlv = (void *)ext_scan_resp->tlv_buffer; - buf_left = le16_to_cpu(resp->size) - (sizeof(*ext_scan_resp) + S_DS_GEN - - 1); + buf_left = le16_to_cpu(resp->size) - (sizeof(*ext_scan_resp) + S_DS_GEN); while (buf_left >= sizeof(struct mwifiex_ie_types_header)) { type = le16_to_cpu(tlv->type); -- Gustavo
On Wed, Aug 21, 2024 at 02:59:34PM -0600, Gustavo A. R. Silva wrote: > On 21/08/24 14:26, Andy Shevchenko wrote: > > On Thu, Feb 02, 2023 at 07:32:00PM -0600, Gustavo A. R. Silva wrote: > > > One-element arrays are deprecated, and we are replacing them with flexible > > > array members instead. So, replace one-element arrays with flexible-array > > > members in multiple structures. > > > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > > routines on memcpy() and help us make progress towards globally > > > enabling -fstrict-flex-arrays=3 [1]. > > > > > > This results in no differences in binary output. > > > > Sorry for blast from the past, but I have a question here. > > > > This change seems converts many of the flexible arrays in this driver. > > But what's behind this one? > > > > struct host_cmd_ds_802_11_scan_ext { > > u32 reserved; > > u8 tlv_buffer[1]; > > } __packed; > > > > > > AFAIU this needs also some care. On the real machine I have got this > > > > elo 16 17:51:58 surfacebook kernel: ------------[ cut here ]------------ > > elo 16 17:51:58 surfacebook kernel: memcpy: detected field-spanning write (size 243) of single field "ext_scan->tlv_buffer" at drivers/net/wireless/marvell/mwifiex/scan.c:2239 (size 1) > > elo 16 17:51:58 surfacebook kernel: WARNING: CPU: 0 PID: 498 at drivers/net/wireless/marvell/mwifiex/scan.c:2239 mwifiex_cmd_802_11_scan_ext+0x83/0x90 [mwifiex] > > > > which leads to > > > > memcpy(ext_scan->tlv_buffer, scan_cfg->tlv_buf, scan_cfg->tlv_buf_len); > > > > but the code allocates 2k or more for the command buffer, so this seems > > quite enough for 243 bytes. > > > > I think this would do it: Thank you for the prompt respond! Can you send it as a formal patch? Or do you want me to test it first? (If the second one, it might take weeks as this is my home laptop that I don't reboot too often. I think it's can be sent anyway.)
On 21/08/24 15:06, Andy Shevchenko wrote: > On Wed, Aug 21, 2024 at 02:59:34PM -0600, Gustavo A. R. Silva wrote: >> On 21/08/24 14:26, Andy Shevchenko wrote: >>> On Thu, Feb 02, 2023 at 07:32:00PM -0600, Gustavo A. R. Silva wrote: >>>> One-element arrays are deprecated, and we are replacing them with flexible >>>> array members instead. So, replace one-element arrays with flexible-array >>>> members in multiple structures. >>>> >>>> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE >>>> routines on memcpy() and help us make progress towards globally >>>> enabling -fstrict-flex-arrays=3 [1]. >>>> >>>> This results in no differences in binary output. >>> >>> Sorry for blast from the past, but I have a question here. >>> >>> This change seems converts many of the flexible arrays in this driver. >>> But what's behind this one? >>> >>> struct host_cmd_ds_802_11_scan_ext { >>> u32 reserved; >>> u8 tlv_buffer[1]; >>> } __packed; >>> >>> >>> AFAIU this needs also some care. On the real machine I have got this >>> >>> elo 16 17:51:58 surfacebook kernel: ------------[ cut here ]------------ >>> elo 16 17:51:58 surfacebook kernel: memcpy: detected field-spanning write (size 243) of single field "ext_scan->tlv_buffer" at drivers/net/wireless/marvell/mwifiex/scan.c:2239 (size 1) >>> elo 16 17:51:58 surfacebook kernel: WARNING: CPU: 0 PID: 498 at drivers/net/wireless/marvell/mwifiex/scan.c:2239 mwifiex_cmd_802_11_scan_ext+0x83/0x90 [mwifiex] >>> >>> which leads to >>> >>> memcpy(ext_scan->tlv_buffer, scan_cfg->tlv_buf, scan_cfg->tlv_buf_len); >>> >>> but the code allocates 2k or more for the command buffer, so this seems >>> quite enough for 243 bytes. >>> >> >> I think this would do it: > > Thank you for the prompt respond! Can you send it as a formal patch? > Or do you want me to test it first? (If the second one, it might take > weeks as this is my home laptop that I don't reboot too often. I think > it's can be sent anyway.) > Done: https://lore.kernel.org/linux-hardening/ZsZa5xRcsLq9D+RX@elsanto/ Thanks for reporting this. :) -- Gustavo
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index b4f945a549f7..9616bd8b49f1 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -41,7 +41,7 @@ struct mwifiex_fw_header { struct mwifiex_fw_data { struct mwifiex_fw_header header; __le32 seq_num; - u8 data[1]; + u8 data[]; } __packed; struct mwifiex_fw_dump_header { @@ -641,7 +641,7 @@ struct mwifiex_ie_types_header { struct mwifiex_ie_types_data { struct mwifiex_ie_types_header header; - u8 data[1]; + u8 data[]; } __packed; #define MWIFIEX_TxPD_POWER_MGMT_NULL_PACKET 0x01 @@ -799,7 +799,7 @@ struct mwifiex_ie_types_rates_param_set { struct mwifiex_ie_types_ssid_param_set { struct mwifiex_ie_types_header header; - u8 ssid[1]; + u8 ssid[]; } __packed; struct mwifiex_ie_types_num_probes { @@ -907,7 +907,7 @@ struct mwifiex_ie_types_tdls_idle_timeout { struct mwifiex_ie_types_rsn_param_set { struct mwifiex_ie_types_header header; - u8 rsn_ie[1]; + u8 rsn_ie[]; } __packed; #define KEYPARAMSET_FIXED_LEN 6 @@ -1433,7 +1433,7 @@ struct mwifiex_tdls_stop_cs_params { struct host_cmd_ds_tdls_config { __le16 tdls_action; - u8 tdls_data[1]; + u8 tdls_data[]; } __packed; struct mwifiex_chan_desc { @@ -1574,13 +1574,13 @@ struct ie_body { struct host_cmd_ds_802_11_scan { u8 bss_mode; u8 bssid[ETH_ALEN]; - u8 tlv_buffer[1]; + u8 tlv_buffer[]; } __packed; struct host_cmd_ds_802_11_scan_rsp { __le16 bss_descript_size; u8 number_of_sets; - u8 bss_desc_and_tlv_buffer[1]; + u8 bss_desc_and_tlv_buffer[]; } __packed; struct host_cmd_ds_802_11_scan_ext { @@ -1596,7 +1596,7 @@ struct mwifiex_ie_types_bss_mode { struct mwifiex_ie_types_bss_scan_rsp { struct mwifiex_ie_types_header header; u8 bssid[ETH_ALEN]; - u8 frame_body[1]; + u8 frame_body[]; } __packed; struct mwifiex_ie_types_bss_scan_info { @@ -1733,7 +1733,7 @@ struct mwifiex_ie_types_local_pwr_constraint { struct mwifiex_ie_types_wmm_param_set { struct mwifiex_ie_types_header header; - u8 wmm_ie[1]; + u8 wmm_ie[]; } __packed; struct mwifiex_ie_types_mgmt_frame { @@ -1959,7 +1959,7 @@ struct host_cmd_tlv_wep_key { struct mwifiex_ie_types_header header; u8 key_index; u8 is_default; - u8 key[1]; + u8 key[]; }; struct host_cmd_tlv_auth_type {
One-element arrays are deprecated, and we are replacing them with flexible array members instead. So, replace one-element arrays with flexible-array members in multiple structures. This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy() and help us make progress towards globally enabling -fstrict-flex-arrays=3 [1]. This results in no differences in binary output. Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/256 Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1] Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/net/wireless/marvell/mwifiex/fw.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)