> -----Original Message-----
> From: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
> Sent: Friday, 13 September 2024 11:00
> To: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>; Korenblit, Miriam Rachel
> <miriam.rachel.korenblit@xxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> hardening@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> > ---
> > .../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@xxxxxxxxx>
