Message-ID: <4caca5e1-c8ea-46dd-a6f2-532030a48454@embeddedor.com>
Date: Fri, 13 Sep 2024 10:00:05 +0200
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: dvm: Avoid
 -Wflex-array-member-not-at-end warnings

Hi all,

Friendly ping: who can take this, please? :)

Thanks
--
Gustavo

On 15/08/24 21:00, 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.
> 
> So, with these changes, fix the following warnings:
> 
> drivers/net/wireless/intel/iwlwifi/dvm/commands.h:2315: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/dvm/commands.h:2426: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/dvm/commands.h | 154 +++++++++---------
>   1 file changed, 78 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/commands.h b/drivers/net/wireless/intel/iwlwifi/dvm/commands.h
> index 3f49c0bccb28..96ea6c8dfc89 100644
> --- a/drivers/net/wireless/intel/iwlwifi/dvm/commands.h
> +++ b/drivers/net/wireless/intel/iwlwifi/dvm/commands.h
> @@ -1180,85 +1180,87 @@ struct iwl_dram_scratch {
>   } __packed;
>   
>   struct iwl_tx_cmd {
> -	/*
> -	 * MPDU byte count:
> -	 * MAC header (24/26/30/32 bytes) + 2 bytes pad if 26/30 header size,
> -	 * + 8 byte IV for CCM or TKIP (not used for WEP)
> -	 * + Data payload
> -	 * + 8-byte MIC (not used for CCM/WEP)
> -	 * NOTE:  Does not include Tx command bytes, post-MAC pad bytes,
> -	 *        MIC (CCM) 8 bytes, ICV (WEP/TKIP/CKIP) 4 bytes, CRC 4 bytes.i
> -	 * Range: 14-2342 bytes.
> -	 */
> -	__le16 len;
> -
> -	/*
> -	 * MPDU or MSDU byte count for next frame.
> -	 * Used for fragmentation and bursting, but not 11n aggregation.
> -	 * Same as "len", but for next frame.  Set to 0 if not applicable.
> -	 */
> -	__le16 next_frame_len;
> -
> -	__le32 tx_flags;	/* TX_CMD_FLG_* */
> -
> -	/* uCode may modify this field of the Tx command (in host DRAM!).
> -	 * Driver must also set dram_lsb_ptr and dram_msb_ptr in this cmd. */
> -	struct iwl_dram_scratch scratch;
> -
> -	/* Rate for *all* Tx attempts, if TX_CMD_FLG_STA_RATE_MSK is cleared. */
> -	__le32 rate_n_flags;	/* RATE_MCS_* */
> -
> -	/* Index of destination station in uCode's station table */
> -	u8 sta_id;
> -
> -	/* Type of security encryption:  CCM or TKIP */
> -	u8 sec_ctl;		/* TX_CMD_SEC_* */
> -
> -	/*
> -	 * Index into rate table (see REPLY_TX_LINK_QUALITY_CMD) for initial
> -	 * Tx attempt, if TX_CMD_FLG_STA_RATE_MSK is set.  Normally "0" for
> -	 * data frames, this field may be used to selectively reduce initial
> -	 * rate (via non-0 value) for special frames (e.g. management), while
> -	 * still supporting rate scaling for all frames.
> -	 */
> -	u8 initial_rate_index;
> -	u8 reserved;
> -	u8 key[16];
> -	__le16 next_frame_flags;
> -	__le16 reserved2;
> -	union {
> -		__le32 life_time;
> -		__le32 attempt;
> -	} stop_time;
> -
> -	/* Host DRAM physical address pointer to "scratch" in this command.
> -	 * Must be dword aligned.  "0" in dram_lsb_ptr disables usage. */
> -	__le32 dram_lsb_ptr;
> -	u8 dram_msb_ptr;
> -
> -	u8 rts_retry_limit;	/*byte 50 */
> -	u8 data_retry_limit;	/*byte 51 */
> -	u8 tid_tspec;
> -	union {
> -		__le16 pm_frame_timeout;
> -		__le16 attempt_duration;
> -	} timeout;
> -
> -	/*
> -	 * Duration of EDCA burst Tx Opportunity, in 32-usec units.
> -	 * Set this if txop time is not specified by HCCA protocol (e.g. by AP).
> -	 */
> -	__le16 driver_txop;
> -
> +	/* New members MUST be added within the __struct_group() macro below. */
> +	__struct_group(iwl_tx_cmd_hdr, __hdr, __packed,
> +		/*
> +		 * MPDU byte count:
> +		 * MAC header (24/26/30/32 bytes) + 2 bytes pad if 26/30 header size,
> +		 * + 8 byte IV for CCM or TKIP (not used for WEP)
> +		 * + Data payload
> +		 * + 8-byte MIC (not used for CCM/WEP)
> +		 * NOTE:  Does not include Tx command bytes, post-MAC pad bytes,
> +		 *        MIC (CCM) 8 bytes, ICV (WEP/TKIP/CKIP) 4 bytes, CRC 4 bytes.i
> +		 * Range: 14-2342 bytes.
> +		 */
> +		__le16 len;
> +
> +		/*
> +		 * MPDU or MSDU byte count for next frame.
> +		 * Used for fragmentation and bursting, but not 11n aggregation.
> +		 * Same as "len", but for next frame.  Set to 0 if not applicable.
> +		 */
> +		__le16 next_frame_len;
> +
> +		__le32 tx_flags;	/* TX_CMD_FLG_* */
> +
> +		/* uCode may modify this field of the Tx command (in host DRAM!).
> +		 * Driver must also set dram_lsb_ptr and dram_msb_ptr in this cmd. */
> +		struct iwl_dram_scratch scratch;
> +
> +		/* Rate for *all* Tx attempts, if TX_CMD_FLG_STA_RATE_MSK is cleared. */
> +		__le32 rate_n_flags;	/* RATE_MCS_* */
> +
> +		/* Index of destination station in uCode's station table */
> +		u8 sta_id;
> +
> +		/* Type of security encryption:  CCM or TKIP */
> +		u8 sec_ctl;		/* TX_CMD_SEC_* */
> +
> +		/*
> +		 * Index into rate table (see REPLY_TX_LINK_QUALITY_CMD) for initial
> +		 * Tx attempt, if TX_CMD_FLG_STA_RATE_MSK is set.  Normally "0" for
> +		 * data frames, this field may be used to selectively reduce initial
> +		 * rate (via non-0 value) for special frames (e.g. management), while
> +		 * still supporting rate scaling for all frames.
> +		 */
> +		u8 initial_rate_index;
> +		u8 reserved;
> +		u8 key[16];
> +		__le16 next_frame_flags;
> +		__le16 reserved2;
> +		union {
> +			__le32 life_time;
> +			__le32 attempt;
> +		} stop_time;
> +
> +		/* Host DRAM physical address pointer to "scratch" in this command.
> +		 * Must be dword aligned.  "0" in dram_lsb_ptr disables usage. */
> +		__le32 dram_lsb_ptr;
> +		u8 dram_msb_ptr;
> +
> +		u8 rts_retry_limit;	/*byte 50 */
> +		u8 data_retry_limit;	/*byte 51 */
> +		u8 tid_tspec;
> +		union {
> +			__le16 pm_frame_timeout;
> +			__le16 attempt_duration;
> +		} timeout;
> +
> +		/*
> +		 * Duration of EDCA burst Tx Opportunity, in 32-usec units.
> +		 * Set this if txop time is not specified by HCCA protocol (e.g. by AP).
> +		 */
> +		__le16 driver_txop;
> +
> +	);
>   	/*
>   	 * MAC header goes here, followed by 2 bytes padding if MAC header
>   	 * length is 26 or 30 bytes, followed by payload data
>   	 */
> -	union {
> -		DECLARE_FLEX_ARRAY(u8, payload);
> -		DECLARE_FLEX_ARRAY(struct ieee80211_hdr, hdr);
> -	};
> +	struct ieee80211_hdr hdr[];
>   } __packed;
> +static_assert(offsetof(struct iwl_tx_cmd, hdr) == sizeof(struct iwl_tx_cmd_hdr),
> +	      "struct member likely outside of __struct_group()");
>   
>   /*
>    * TX command response is sent after *agn* transmission attempts.
> @@ -2312,7 +2314,7 @@ struct iwl_scan_cmd {
>   
>   	/* For active scans (set to all-0s for passive scans).
>   	 * Does not include payload.  Must specify Tx rate; no rate scaling. */
> -	struct iwl_tx_cmd tx_cmd;
> +	struct iwl_tx_cmd_hdr tx_cmd;
>   
>   	/* For directed active scans (set to all-0s otherwise) */
>   	struct iwl_ssid_ie direct_scan[PROBE_OPTION_MAX];
> @@ -2423,7 +2425,7 @@ struct iwlagn_beacon_notif {
>    */
>   
>   struct iwl_tx_beacon_cmd {
> -	struct iwl_tx_cmd tx;
> +	struct iwl_tx_cmd_hdr tx;
>   	__le16 tim_idx;
>   	u8 tim_size;
>   	u8 reserved1;

Powered by blists - more mailing lists