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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e54fb6fb-dd99-407e-84e5-1866d3d9248e@embeddedor.com>
Date: Fri, 4 Oct 2024 13:38:09 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Miri Korenblit <miriam.rachel.korenblit@...el.com>,
 Kalle Valo <kvalo@...nel.org>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] wifi: iwlwifi: fw/mvm: Avoid multiple
 -Wflex-array-member-not-at-end warnings

Hi all,

Friendly ping: who can take this, please? 🙂

Thanks
-Gustavo

On 15/08/24 13:54, 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.
> 
> Worth mentioning is that the union at the end of the flexible structure
> was replaced by a direct declaration of flexible-array member `hdr[]`
> as `payload` is unnecessary.
> 
> So, with these changes, fix the following warnings:
> 
> drivers/net/wireless/intel/iwlwifi/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/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]
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>   .../net/wireless/intel/iwlwifi/fw/api/tdls.h  |  4 +-
>   .../net/wireless/intel/iwlwifi/fw/api/tx.h    | 61 ++++++++++---------
>   .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c |  8 ++-
>   drivers/net/wireless/intel/iwlwifi/mvm/tdls.c |  8 ++-
>   4 files changed, 45 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/api/tdls.h b/drivers/net/wireless/intel/iwlwifi/fw/api/tdls.h
> index 893438aadab0..0ea6c0e37750 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_MVM_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 c5277e2f8cd4..903f0f7517e0 100644
> --- a/drivers/net/wireless/intel/iwlwifi/fw/api/tx.h
> +++ b/drivers/net/wireless/intel/iwlwifi/fw/api/tx.h
> @@ -222,34 +222,37 @@ 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 +745,7 @@ struct iwl_mvm_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 +764,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 dfcc96f18b4f..41e276f2fcf8 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> @@ -1053,8 +1053,10 @@ 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 = {};
> +	struct iwl_tx_cmd *beacon_cmd_tx =
> +		container_of(&beacon_cmd.tx, struct iwl_tx_cmd, __hdr);
>   
> -	iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon, &beacon_cmd.tx);
> +	iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon, beacon_cmd_tx);
>   
>   	beacon_cmd.template_id = cpu_to_le32((u32)mvmvif->id);
>   
> @@ -1073,8 +1075,10 @@ 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 = {};
> +	struct iwl_tx_cmd *beacon_cmd_tx =
> +			container_of(&beacon_cmd.tx, struct iwl_tx_cmd, __hdr);
>   
> -	iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon, &beacon_cmd.tx);
> +	iwl_mvm_mac_ctxt_set_tx(mvm, vif, beacon, beacon_cmd_tx);
>   
>   	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 3d25ff5cd7e8..7e45445c3ce6 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tdls.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tdls.c
> @@ -341,6 +341,8 @@ iwl_mvm_tdls_config_channel_switch(struct iwl_mvm *mvm,
>   	struct iwl_tdls_channel_switch_cmd cmd = {0};
>   	struct iwl_tdls_channel_switch_cmd_tail *tail =
>   		iwl_mvm_chan_info_cmd_tail(mvm, &cmd.ci);
> +	struct iwl_tx_cmd *tail_frame_tx_cmd =
> +		container_of(&tail->frame.tx_cmd, struct iwl_tx_cmd, __hdr);
>   	u16 len = sizeof(cmd) - iwl_mvm_chan_info_padding(mvm);
>   	int ret;
>   
> @@ -410,13 +412,13 @@ 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, tail_frame_tx_cmd);
>   	}
>   
> -	iwl_mvm_set_tx_cmd(mvm, skb, &tail->frame.tx_cmd, info,
> +	iwl_mvm_set_tx_cmd(mvm, skb, tail_frame_tx_cmd, info,
>   			   mvmsta->deflink.sta_id);
>   
> -	iwl_mvm_set_tx_cmd_rate(mvm, &tail->frame.tx_cmd, info, sta,
> +	iwl_mvm_set_tx_cmd_rate(mvm, tail_frame_tx_cmd, info, sta,
>   				hdr->frame_control);
>   	rcu_read_unlock();
>   

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ