| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1ef9857b-efdb-4142-b068-91cb453849ad@embeddedor.com>
Date: Tue, 13 May 2025 13:14:07 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
Miri Korenblit <miriam.rachel.korenblit@...el.com>,
Johannes Berg <johannes.berg@...el.com>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org, Kees Cook <kees@...nel.org>
Subject: Re: [PATCH v2][next] wifi: iwlwifi: mvm/fw: Avoid
-Wflex-array-member-not-at-end warnings
Hi all,
Friendly ping: who can take this, please? :)
Thanks!
-Gustavo
On 25/04/25 15:14, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally.
>
> So, in order to avoid ending up with a flexible-array member in the
> middle of multiple other structs, we use the `__struct_group()`
> helper to create a new tagged `struct iwl_tx_cmd_hdr`. This structure
> groups together all the members of the flexible `struct iwl_tx_cmd`
> except the flexible array.
>
> As a result, the array is effectively separated from the rest of the
> members without modifying the memory layout of the flexible structure.
> We then change the type of the middle struct members currently causing
> trouble from `struct iwl_tx_cmd` to `struct iwl_tx_cmd_hdr`.
>
> We also want to ensure that when new members need to be added to the
> flexible structure, they are always included within the newly created
> tagged struct. For this, we use `static_assert()`. This ensures that the
> memory layout for both the flexible structure and the new tagged struct
> is the same after any changes.
>
> This approach avoids having to implement `struct iwl_tx_cmd_hdr`
> as a completely separate structure, thus preventing having to maintain
> two independent but basically identical structures, closing the door
> to potential bugs in the future.
>
> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure, through which we can access the flexible-array
> member, if necessary.
>
> Also, as part of the refactoring required to use `__struct_group()`,
> remove unused flex array `payload`.
>
> So, with these changes, fix the following warnings:
>
> drivers/net/wireless/intel/iwlwifi/mld/../fw/api/tdls.h:134:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/intel/iwlwifi/mld/../fw/api/tdls.h:53:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/intel/iwlwifi/mld/../fw/api/tx.h:745:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/intel/iwlwifi/mld/../fw/api/tx.h:764:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/intel/iwlwifi/mvm/../fw/api/tdls.h:134:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/intel/iwlwifi/mvm/../fw/api/tdls.h:53:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/intel/iwlwifi/mvm/../fw/api/tx.h:745:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/intel/iwlwifi/mvm/../fw/api/tx.h:764:27: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>
> Reviewed-by: Kees Cook <kees@...nel.org>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
> Changes in v2:
> - Update changelog text - mention removal of flex array `payload`.
> (Kees)
> - Fix kernel-doc block. (Johannes).
> - Add RB tag.
>
> v1:
> - Link: https://lore.kernel.org/linux-hardening/Z-9G-GHufhXKeYft@kspp/
>
> .../net/wireless/intel/iwlwifi/fw/api/tdls.h | 4 +-
> .../net/wireless/intel/iwlwifi/fw/api/tx.h | 63 ++++++++++---------
> .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 8 ++-
> drivers/net/wireless/intel/iwlwifi/mvm/tdls.c | 10 +--
> 4 files changed, 45 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/tdls.h b/drivers/net/wireless/intel/iwlwifi/fw/api/tdls.h
> index cfa6532a3cdd..02198bc37f8c 100644
> --- a/drivers/net/wireless/intel/iwlwifi/fw/api/tdls.h
> +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/tdls.h
> @@ -50,7 +50,7 @@ struct iwl_tdls_channel_switch_timing {
> */
> struct iwl_tdls_channel_switch_frame {
> __le32 switch_time_offset;
> - struct iwl_tx_cmd tx_cmd;
> + struct iwl_tx_cmd_hdr tx_cmd;
> u8 data[IWL_TDLS_CH_SW_FRAME_MAX_SIZE];
> } __packed; /* TDLS_STA_CHANNEL_SWITCH_FRAME_API_S_VER_1 */
>
> @@ -131,7 +131,7 @@ struct iwl_tdls_config_cmd {
> struct iwl_tdls_sta_info sta_info[IWL_TDLS_STA_COUNT];
>
> __le32 pti_req_data_offset;
> - struct iwl_tx_cmd pti_req_tx_cmd;
> + struct iwl_tx_cmd_hdr pti_req_tx_cmd;
> u8 pti_req_template[];
> } __packed; /* TDLS_CONFIG_CMD_API_S_VER_1 */
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/tx.h b/drivers/net/wireless/intel/iwlwifi/fw/api/tx.h
> index 0a39e4b6eb62..dba18d898ea2 100644
> --- a/drivers/net/wireless/intel/iwlwifi/fw/api/tx.h
> +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/tx.h
> @@ -205,8 +205,7 @@ enum iwl_tx_offload_assist_flags_pos {
> * @tid_tspec: TID/tspec
> * @pm_frame_timeout: PM TX frame timeout
> * @reserved4: reserved
> - * @payload: payload (same as @hdr)
> - * @hdr: 802.11 header (same as @payload)
> + * @hdr: 802.11 header
> *
> * The byte count (both len and next_frame_len) includes MAC header
> * (24/26/30/32 bytes)
> @@ -222,34 +221,36 @@ enum iwl_tx_offload_assist_flags_pos {
> * and then the actial payload.
> */
> struct iwl_tx_cmd {
> - __le16 len;
> - __le16 offload_assist;
> - __le32 tx_flags;
> - struct {
> - u8 try_cnt;
> - u8 btkill_cnt;
> - __le16 reserved;
> - } scratch; /* DRAM_SCRATCH_API_U_VER_1 */
> - __le32 rate_n_flags;
> - u8 sta_id;
> - u8 sec_ctl;
> - u8 initial_rate_index;
> - u8 reserved2;
> - u8 key[16];
> - __le32 reserved3;
> - __le32 life_time;
> - __le32 dram_lsb_ptr;
> - u8 dram_msb_ptr;
> - u8 rts_retry_limit;
> - u8 data_retry_limit;
> - u8 tid_tspec;
> - __le16 pm_frame_timeout;
> - __le16 reserved4;
> - union {
> - DECLARE_FLEX_ARRAY(u8, payload);
> - DECLARE_FLEX_ARRAY(struct ieee80211_hdr, hdr);
> - };
> + /* New members MUST be added within the __struct_group() macro below. */
> + __struct_group(iwl_tx_cmd_hdr, __hdr, __packed,
> + __le16 len;
> + __le16 offload_assist;
> + __le32 tx_flags;
> + struct {
> + u8 try_cnt;
> + u8 btkill_cnt;
> + __le16 reserved;
> + } scratch; /* DRAM_SCRATCH_API_U_VER_1 */
> + __le32 rate_n_flags;
> + u8 sta_id;
> + u8 sec_ctl;
> + u8 initial_rate_index;
> + u8 reserved2;
> + u8 key[16];
> + __le32 reserved3;
> + __le32 life_time;
> + __le32 dram_lsb_ptr;
> + u8 dram_msb_ptr;
> + u8 rts_retry_limit;
> + u8 data_retry_limit;
> + u8 tid_tspec;
> + __le16 pm_frame_timeout;
> + __le16 reserved4;
> + );
> + struct ieee80211_hdr hdr[];
> } __packed; /* TX_CMD_API_S_VER_6 */
> +static_assert(offsetof(struct iwl_tx_cmd, hdr) == sizeof(struct iwl_tx_cmd_hdr),
> + "struct member likely outside of __struct_group()");
>
> struct iwl_dram_sec_info {
> __le32 pn_low;
> @@ -742,7 +743,7 @@ struct iwl_compressed_ba_notif {
> * @frame: the template of the beacon frame
> */
> struct iwl_mac_beacon_cmd_v6 {
> - struct iwl_tx_cmd tx;
> + struct iwl_tx_cmd_hdr tx;
> __le32 template_id;
> __le32 tim_idx;
> __le32 tim_size;
> @@ -761,7 +762,7 @@ struct iwl_mac_beacon_cmd_v6 {
> * @frame: the template of the beacon frame
> */
> struct iwl_mac_beacon_cmd_v7 {
> - struct iwl_tx_cmd tx;
> + struct iwl_tx_cmd_hdr tx;
> __le32 template_id;
> __le32 tim_idx;
> __le32 tim_size;
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> index bec18d197f31..f010e68b4a55 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> @@ -1032,7 +1032,9 @@ static int iwl_mvm_mac_ctxt_send_beacon_v6(struct iwl_mvm *mvm,
> struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
> struct iwl_mac_beacon_cmd_v6 beacon_cmd = {};
>
> - iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon, &beacon_cmd.tx);
> + iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon,
> + container_of(&beacon_cmd.tx,
> + struct iwl_tx_cmd, __hdr));
>
> beacon_cmd.template_id = cpu_to_le32((u32)mvmvif->id);
>
> @@ -1052,7 +1054,9 @@ static int iwl_mvm_mac_ctxt_send_beacon_v7(struct iwl_mvm *mvm,
> struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
> struct iwl_mac_beacon_cmd_v7 beacon_cmd = {};
>
> - iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon, &beacon_cmd.tx);
> + iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon,
> + container_of(&beacon_cmd.tx,
> + struct iwl_tx_cmd, __hdr));
>
> beacon_cmd.template_id = cpu_to_le32((u32)mvmvif->id);
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tdls.c b/drivers/net/wireless/intel/iwlwifi/mvm/tdls.c
> index 36379b738de1..bcfae05192ad 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tdls.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tdls.c
> @@ -342,6 +342,8 @@ iwl_mvm_tdls_config_channel_switch(struct iwl_mvm *mvm,
> struct iwl_tdls_channel_switch_cmd_tail *tail =
> iwl_mvm_chan_info_cmd_tail(mvm, &cmd.ci);
> u16 len = sizeof(cmd) - iwl_mvm_chan_info_padding(mvm);
> + struct iwl_tx_cmd *tx_cmd =
> + container_of(&tail->frame.tx_cmd, struct iwl_tx_cmd, __hdr);
> int ret;
>
> lockdep_assert_held(&mvm->mutex);
> @@ -410,14 +412,12 @@ iwl_mvm_tdls_config_channel_switch(struct iwl_mvm *mvm,
> ret = -EINVAL;
> goto out;
> }
> - iwl_mvm_set_tx_cmd_ccmp(info, &tail->frame.tx_cmd);
> + iwl_mvm_set_tx_cmd_ccmp(info, tx_cmd);
> }
>
> - iwl_mvm_set_tx_cmd(mvm, skb, &tail->frame.tx_cmd, info,
> - mvmsta->deflink.sta_id);
> + iwl_mvm_set_tx_cmd(mvm, skb, tx_cmd, info, mvmsta->deflink.sta_id);
>
> - iwl_mvm_set_tx_cmd_rate(mvm, &tail->frame.tx_cmd, info, sta,
> - hdr->frame_control);
> + iwl_mvm_set_tx_cmd_rate(mvm, tx_cmd, info, sta, hdr->frame_control);
> rcu_read_unlock();
>
> memcpy(tail->frame.data, skb->data, skb->len);
Powered by blists - more mailing lists