Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 23 Jun 2023 16:07:08 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Arnd Bergmann <arnd@kernel.org>
Cc:     Johannes Berg <johannes@sipsolutions.net>,
        Arnd Bergmann <arnd@arndb.de>,
        Christian Lamparter <chunkeey@googlemail.com>,
        Kalle Valo <kvalo@kernel.org>,
        Johannes Berg <johannes.berg@intel.com>,
        linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Gregory Greenman <gregory.greenman@intel.com>,
        Benjamin Berg <benjamin.berg@intel.com>,
        Ryder Lee <ryder.lee@mediatek.com>,
        Ilan Peer <ilan.peer@intel.com>, Felix Fietkau <nbd@nbd.name>,
        Aloka Dixit <quic_alokad@quicinc.com>,
        Avraham Stern <avraham.stern@intel.com>,
        Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        netdev@vger.kernel.org
Subject: Re: [PATCH 2/2] mac80211: make ieee80211_tx_info padding explicit
Message-ID: <202306231604.1D327AFE9@keescook>
References: <20230623152443.2296825-1-arnd@kernel.org>
 <20230623152443.2296825-2-arnd@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230623152443.2296825-2-arnd@kernel.org>
Precedence: bulk

On Fri, Jun 23, 2023 at 05:24:00PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> While looking at a bug, I got rather confused by the layout of the
> 'status' field in ieee80211_tx_info. Apparently, the intention is that
> status_driver_data[] is used for driver specific data, and fills up the
> size of the union to 40 bytes, just like the other ones.
> 
> This is indeed what actually happens, but only because of the
> combination of two mistakes:
> 
>  - "void *status_driver_data[18 / sizeof(void *)];" is intended
>    to be 18 bytes long but is actually two bytes shorter because of
>    rounding-down in the division, to a multiple of the pointer
>    size (4 bytes or 8 bytes).
> 
>  - The other fields combined are intended to be 22 bytes long, but
>    are actually 24 bytes because of padding in front of the
>    unaligned tx_time member, and in front of the pointer array.
> 
> The two mistakes cancel out. so the size ends up fine, but it seems
> more helpful to make this explicit, by having a multiple of 8 bytes
> in the size calculation and explicitly describing the padding.
> 
> Fixes: ea5907db2a9cc ("mac80211: fix struct ieee80211_tx_info size")
> Fixes: 02219b3abca59 ("mac80211: add WMM admission control support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/net/mac80211.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 3a8a2d2c58c38..ca4dc8a14f1bb 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1192,9 +1192,11 @@ struct ieee80211_tx_info {
>  			u8 ampdu_ack_len;
>  			u8 ampdu_len;
>  			u8 antenna;
> +			u8 pad;
>  			u16 tx_time;
>  			u8 flags;
> -			void *status_driver_data[18 / sizeof(void *)];
> +			u8 pad2;
> +			void *status_driver_data[16 / sizeof(void *)];
>  		} status;

pahole agrees with your assessment. :)

struct ieee80211_tx_info {
	...
        union {
		...
                struct {
                        struct ieee80211_tx_rate rates[4]; /*     8    12 */
                        s32        ack_signal;           /*    20     4 */
                        u8         ampdu_ack_len;        /*    24     1 */
                        u8         ampdu_len;            /*    25     1 */
                        u8         antenna;              /*    26     1 */

                        /* XXX 1 byte hole, try to pack */

                        u16        tx_time;              /*    28     2 */
                        u8         flags;                /*    30     1 */

                        /* XXX 1 byte hole, try to pack */

                        void *     status_driver_data[2]; /*    32    16 */
                } status;                                /*     8    40 */
                struct {
                        struct ieee80211_tx_rate driver_rates[4]; /*     8    12 */
                        u8         pad[4];               /*    20     4 */
                        void *     rate_driver_data[3];  /*    24    24 */
                };                                       /*     8    40 */
                void *             driver_data[5];       /*     8    40 */
        };                                               /*     8    40 */
	...
};

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook