Re: [PATCH 2/2] mac80211: make ieee80211_tx_info padding explicit

[Kees Cook]

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

-- 
Kees Cook