linux-kernel - Re: [PATCH 2/2] mac80211: make ieee80211_tx

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: <202306231604.1D327AFE9@keescook>
Date:   Fri, 23 Jun 2023 16:07:08 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Johannes Berg <johannes@...solutions.net>,
        Arnd Bergmann <arnd@...db.de>,
        Christian Lamparter <chunkeey@...glemail.com>,
        Kalle Valo <kvalo@...nel.org>,
        Johannes Berg <johannes.berg@...el.com>,
        linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Gregory Greenman <gregory.greenman@...el.com>,
        Benjamin Berg <benjamin.berg@...el.com>,
        Ryder Lee <ryder.lee@...iatek.com>,
        Ilan Peer <ilan.peer@...el.com>, Felix Fietkau <nbd@....name>,
        Aloka Dixit <quic_alokad@...cinc.com>,
        Avraham Stern <avraham.stern@...el.com>,
        Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] mac80211: make ieee80211_tx_info padding explicit

On Fri, Jun 23, 2023 at 05:24:00PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.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@...db.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@...omium.org>

-- 
Kees Cook