Message-ID: <75551003-17c7-450a-89b0-818b6a01051c@embeddedor.com>
Date: Mon, 10 Mar 2025 14:17:50 +1030
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,

I wonder who can take this patch, please. :)

It was submitted around the same time as this other one:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a7e8997ae18c42d30bc7181421b5715e319c0f71

And for some reason it wasn't taken back then.

If you have any comments, please let me know.

Thanks!
--
Gustavo

On 05/10/24 05:09, Gustavo A. R. Silva wrote:
> Hi all,
> 
> Friendly ping: who can take this, please? 🙂
> 
> Thanks
> -Gustavo
> 
> On 15/08/24 13: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