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: <MW5PR11MB5810CE842DDCA8B2EA4A819EA3DC2@MW5PR11MB5810.namprd11.prod.outlook.com>
Date: Sun, 16 Mar 2025 12:48:45 +0000
From: "Korenblit, Miriam Rachel" <miriam.rachel.korenblit@...el.com>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>, "Gustavo A. R. Silva"
	<gustavoars@...nel.org>, Kalle Valo <kvalo@...nel.org>
CC: "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: RE: [PATCH][next] wifi: iwlwifi: dvm: Avoid
 -Wflex-array-member-not-at-end warnings



> -----Original Message-----
> From: Gustavo A. R. Silva <gustavo@...eddedor.com>
> Sent: Friday, 13 September 2024 11:00
> To: Gustavo A. R. Silva <gustavoars@...nel.org>; Korenblit, Miriam Rachel
> <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;

This sub driver is not supported anymore so I believe that the static assert it not needed,
But can't harm :)

Acked-by: Miri Korenblit <miriam.rachel.korenblit@...el.com>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ