linux-hardening - Re: [PATCH][next] wifi: ath11k: Avoid -Wflex-array-member-not-at-end warnings

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8d31adac-fd43-4cf9-8fc8-655b359a573c@quicinc.com>
Date: Fri, 9 Aug 2024 19:21:10 -0700
From: Jeff Johnson <quic_jjohnson@...cinc.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Kalle Valo
	<kvalo@...nel.org>, Jeff Johnson <jjohnson@...nel.org>
CC: <linux-wireless@...r.kernel.org>, <ath11k@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-hardening@...r.kernel.org>
Subject: Re: [PATCH][next] wifi: ath11k: Avoid -Wflex-array-member-not-at-end
 warnings

On 8/9/2024 9:20 AM, 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.
> 
> Move the conflicting declaration to the end of the structure. Notice
> that `struct ieee80211_chanctx_conf` is a flexible structure --a
> structure that contains a flexible-array member.
> 
> Also, remove a couple of unused structures.
> 
> Fix the following warnings:
> drivers/net/wireless/ath/ath11k/core.h:409:39: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/ath/ath11k/dp.h:1309:24: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/net/wireless/ath/ath11k/dp.h:1368:24: 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>
> ---
>  drivers/net/wireless/ath/ath11k/core.h |  4 +++-
>  drivers/net/wireless/ath/ath11k/dp.h   | 23 -----------------------
>  2 files changed, 3 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
> index df24f0e409af..e283415dccf3 100644
> --- a/drivers/net/wireless/ath/ath11k/core.h
> +++ b/drivers/net/wireless/ath/ath11k/core.h
> @@ -406,11 +406,13 @@ struct ath11k_vif {
>  	bool wpaie_present;
>  	bool bcca_zero_sent;
>  	bool do_not_send_tmpl;
> -	struct ieee80211_chanctx_conf chanctx;
>  	struct ath11k_arp_ns_offload arp_ns_offload;
>  	struct ath11k_rekey_data rekey_data;
>  
>  	struct ath11k_reg_tpc_power_info reg_tpc_info;
> +
> +	/* Must be last - ends in a flexible-array member. */
> +	struct ieee80211_chanctx_conf chanctx;

there is something illogical about this since the vif is allocated using
sizeof() and hence there will never be memory allocated for the flexible
array, and it is assigned using either struct assignment or memcpy using the
struct size which (fortunately) would not transfer the flexible array contents:
		arvif->chanctx = *ctx;

		memcpy(&arvif->chanctx, ctx, sizeof(*ctx));

since ath11k doesn't actually use the drv_priv[] I guess this change is OK, it
is just strange to me.

also makes me wonder why ath11k keeps a copy of the chanctx instead of just
getting it from the underlying ieee80211_link_data. but that is outside the
scope of this discussion.

>  };
>  
>  struct ath11k_vif_iter {
> diff --git a/drivers/net/wireless/ath/ath11k/dp.h b/drivers/net/wireless/ath/ath11k/dp.h
> index 2f6dd69d3be2..65d2bc0687c8 100644
> --- a/drivers/net/wireless/ath/ath11k/dp.h
> +++ b/drivers/net/wireless/ath/ath11k/dp.h
> @@ -1305,18 +1305,6 @@ struct htt_ppdu_stats_user_rate {
>  #define HTT_TX_INFO_PEERID(_flags) \
>  			FIELD_GET(HTT_PPDU_STATS_TX_INFO_FLAGS_PEERID_M, _flags)
>  
> -struct htt_tx_ppdu_stats_info {
> -	struct htt_tlv tlv_hdr;
> -	u32 tx_success_bytes;
> -	u32 tx_retry_bytes;
> -	u32 tx_failed_bytes;
> -	u32 flags; /* %HTT_PPDU_STATS_TX_INFO_FLAGS_ */
> -	u16 tx_success_msdus;
> -	u16 tx_retry_msdus;
> -	u16 tx_failed_msdus;
> -	u16 tx_duration; /* united in us */
> -} __packed;
> -
>  enum  htt_ppdu_stats_usr_compln_status {
>  	HTT_PPDU_STATS_USER_STATUS_OK,
>  	HTT_PPDU_STATS_USER_STATUS_FILTERED,
> @@ -1364,17 +1352,6 @@ struct htt_ppdu_stats_usr_cmpltn_ack_ba_status {
>  	u32 success_bytes;
>  } __packed;
>  
> -struct htt_ppdu_stats_usr_cmn_array {
> -	struct htt_tlv tlv_hdr;
> -	u32 num_ppdu_stats;
> -	/* tx_ppdu_stats_info is filled by multiple struct htt_tx_ppdu_stats_info
> -	 * elements.
> -	 * tx_ppdu_stats_info is variable length, with length =
> -	 *     number_of_ppdu_stats * sizeof (struct htt_tx_ppdu_stats_info)
> -	 */
> -	struct htt_tx_ppdu_stats_info tx_ppdu_info[];
> -} __packed;
> -
>  struct htt_ppdu_user_stats {
>  	u16 peer_id;
>  	u32 tlv_flags;

the second part if definitely ok.

Acked-by: Jeff Johnson <quic_jjohnson@...cinc.com>