| Message ID | Y9xkjXeElSEQ0FPY@work |
|---|---|
| State | New |
| Headers | show |
| Series | [next] wifi: mwifiex: Replace one-element array with flexible-array member | expand |
On Thu, Feb 02, 2023 at 07:34:05PM -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 array with flexible-array > member in struct mwifiex_ie_types_rates_param_set. > > These are the only binary differences I see after the change: > > mwifiex.o > _@@ -50154,7 +50154,7 @@ > 23514: R_X86_64_32S kmalloc_caches+0x50 > 23518: call 2351d <mwifiex_scan_networks+0x11d> > 23519: R_X86_64_PLT32 __tsan_read8-0x4 > - 2351d: mov $0x225,%edx > + 2351d: mov $0x224,%edx > 23522: mov $0xdc0,%esi > 23527: mov 0x0(%rip),%rdi # 2352e <mwifiex_scan_networks+0x12e> > 2352a: R_X86_64_PC32 kmalloc_caches+0x4c > scan.o > _@@ -5582,7 +5582,7 @@ > 4394: R_X86_64_32S kmalloc_caches+0x50 > 4398: call 439d <mwifiex_scan_networks+0x11d> > 4399: R_X86_64_PLT32 __tsan_read8-0x4 > - 439d: mov $0x225,%edx > + 439d: mov $0x224,%edx > 43a2: mov $0xdc0,%esi > 43a7: mov 0x0(%rip),%rdi # 43ae <mwifiex_scan_networks+0x12e> > 43aa: R_X86_64_PC32 kmalloc_caches+0x4c > > and the reason for that is the following line: > > drivers/net/wireless/marvell/mwifiex/scan.c: > 1517 scan_cfg_out = kzalloc(sizeof(union mwifiex_scan_cmd_config_tlv), > 1518 GFP_KERNEL); > > sizeof(union mwifiex_scan_cmd_config_tlv) is now one-byte smaller due to the > flex-array transformation: > > 46 union mwifiex_scan_cmd_config_tlv { > 47 /* Scan configuration (variable length) */ > 48 struct mwifiex_scan_cmd_config config; > 49 /* Max allocated block */ > 50 u8 config_alloc_buf[MAX_SCAN_CFG_ALLOC]; > 51 }; Interesting! So this looks like it's fixing a minor bug in the original implementation which was allocation 1 byte too much. > > Notice that MAX_SCAN_CFG_ALLOC is defined in terms of > sizeof(struct mwifiex_ie_types_rates_param_set), see: > > 26 /* Memory needed to store supported rate */ > 27 #define RATE_TLV_MAX_SIZE (sizeof(struct mwifiex_ie_types_rates_param_set) \ > 28 + HOSTCMD_SUPPORTED_RATES) > > 37 /* Maximum memory needed for a mwifiex_scan_cmd_config with all TLVs at max */ > 38 #define MAX_SCAN_CFG_ALLOC (sizeof(struct mwifiex_scan_cmd_config) \ > 39 + sizeof(struct mwifiex_ie_types_num_probes) \ > 40 + sizeof(struct mwifiex_ie_types_htcap) \ > 41 + CHAN_TLV_MAX_SIZE \ > 42 + RATE_TLV_MAX_SIZE \ > 43 + WILDCARD_SSID_TLV_MAX_SIZE) Yeah, the config_alloc_buf size appears to be very specifically calculated, so this seems sane to me. > > 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]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/252 > 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 array with flexible-array > member in struct mwifiex_ie_types_rates_param_set. > > These are the only binary differences I see after the change: > > mwifiex.o > _@@ -50154,7 +50154,7 @@ > 23514: R_X86_64_32S kmalloc_caches+0x50 > 23518: call 2351d <mwifiex_scan_networks+0x11d> > 23519: R_X86_64_PLT32 __tsan_read8-0x4 > - 2351d: mov $0x225,%edx > + 2351d: mov $0x224,%edx > 23522: mov $0xdc0,%esi > 23527: mov 0x0(%rip),%rdi # 2352e <mwifiex_scan_networks+0x12e> > 2352a: R_X86_64_PC32 kmalloc_caches+0x4c > scan.o > _@@ -5582,7 +5582,7 @@ > 4394: R_X86_64_32S kmalloc_caches+0x50 > 4398: call 439d <mwifiex_scan_networks+0x11d> > 4399: R_X86_64_PLT32 __tsan_read8-0x4 > - 439d: mov $0x225,%edx > + 439d: mov $0x224,%edx > 43a2: mov $0xdc0,%esi > 43a7: mov 0x0(%rip),%rdi # 43ae <mwifiex_scan_networks+0x12e> > 43aa: R_X86_64_PC32 kmalloc_caches+0x4c > > and the reason for that is the following line: > > drivers/net/wireless/marvell/mwifiex/scan.c: > 1517 scan_cfg_out = kzalloc(sizeof(union mwifiex_scan_cmd_config_tlv), > 1518 GFP_KERNEL); > > sizeof(union mwifiex_scan_cmd_config_tlv) is now one-byte smaller due to the > flex-array transformation: > > 46 union mwifiex_scan_cmd_config_tlv { > 47 /* Scan configuration (variable length) */ > 48 struct mwifiex_scan_cmd_config config; > 49 /* Max allocated block */ > 50 u8 config_alloc_buf[MAX_SCAN_CFG_ALLOC]; > 51 }; > > Notice that MAX_SCAN_CFG_ALLOC is defined in terms of > sizeof(struct mwifiex_ie_types_rates_param_set), see: > > 26 /* Memory needed to store supported rate */ > 27 #define RATE_TLV_MAX_SIZE (sizeof(struct mwifiex_ie_types_rates_param_set) \ > 28 + HOSTCMD_SUPPORTED_RATES) > > 37 /* Maximum memory needed for a mwifiex_scan_cmd_config with all TLVs at max */ > 38 #define MAX_SCAN_CFG_ALLOC (sizeof(struct mwifiex_scan_cmd_config) \ > 39 + sizeof(struct mwifiex_ie_types_num_probes) \ > 40 + sizeof(struct mwifiex_ie_types_htcap) \ > 41 + CHAN_TLV_MAX_SIZE \ > 42 + RATE_TLV_MAX_SIZE \ > 43 + WILDCARD_SSID_TLV_MAX_SIZE) > > 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]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/252 > 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. 235fd607c6cb wifi: mwifiex: Replace one-element array with flexible-array member
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 9616bd8b49f1..8c7c744683bc 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -794,7 +794,7 @@ struct mwifiex_ie_types_chan_band_list_param_set { struct mwifiex_ie_types_rates_param_set { struct mwifiex_ie_types_header header; - u8 rates[1]; + u8 rates[]; } __packed; struct mwifiex_ie_types_ssid_param_set {
One-element arrays are deprecated, and we are replacing them with flexible array members instead. So, replace one-element array with flexible-array member in struct mwifiex_ie_types_rates_param_set. These are the only binary differences I see after the change: mwifiex.o _@@ -50154,7 +50154,7 @@ 23514: R_X86_64_32S kmalloc_caches+0x50 23518: call 2351d <mwifiex_scan_networks+0x11d> 23519: R_X86_64_PLT32 __tsan_read8-0x4 - 2351d: mov $0x225,%edx + 2351d: mov $0x224,%edx 23522: mov $0xdc0,%esi 23527: mov 0x0(%rip),%rdi # 2352e <mwifiex_scan_networks+0x12e> 2352a: R_X86_64_PC32 kmalloc_caches+0x4c scan.o _@@ -5582,7 +5582,7 @@ 4394: R_X86_64_32S kmalloc_caches+0x50 4398: call 439d <mwifiex_scan_networks+0x11d> 4399: R_X86_64_PLT32 __tsan_read8-0x4 - 439d: mov $0x225,%edx + 439d: mov $0x224,%edx 43a2: mov $0xdc0,%esi 43a7: mov 0x0(%rip),%rdi # 43ae <mwifiex_scan_networks+0x12e> 43aa: R_X86_64_PC32 kmalloc_caches+0x4c and the reason for that is the following line: drivers/net/wireless/marvell/mwifiex/scan.c: 1517 scan_cfg_out = kzalloc(sizeof(union mwifiex_scan_cmd_config_tlv), 1518 GFP_KERNEL); sizeof(union mwifiex_scan_cmd_config_tlv) is now one-byte smaller due to the flex-array transformation: 46 union mwifiex_scan_cmd_config_tlv { 47 /* Scan configuration (variable length) */ 48 struct mwifiex_scan_cmd_config config; 49 /* Max allocated block */ 50 u8 config_alloc_buf[MAX_SCAN_CFG_ALLOC]; 51 }; Notice that MAX_SCAN_CFG_ALLOC is defined in terms of sizeof(struct mwifiex_ie_types_rates_param_set), see: 26 /* Memory needed to store supported rate */ 27 #define RATE_TLV_MAX_SIZE (sizeof(struct mwifiex_ie_types_rates_param_set) \ 28 + HOSTCMD_SUPPORTED_RATES) 37 /* Maximum memory needed for a mwifiex_scan_cmd_config with all TLVs at max */ 38 #define MAX_SCAN_CFG_ALLOC (sizeof(struct mwifiex_scan_cmd_config) \ 39 + sizeof(struct mwifiex_ie_types_num_probes) \ 40 + sizeof(struct mwifiex_ie_types_htcap) \ 41 + CHAN_TLV_MAX_SIZE \ 42 + RATE_TLV_MAX_SIZE \ 43 + WILDCARD_SSID_TLV_MAX_SIZE) 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]. Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/252 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)