* Re: [PATCH 2/2] mac80211: make ieee80211_tx_info padding explicit
2023-06-23 15:24 ` [PATCH 2/2] mac80211: make ieee80211_tx_info padding explicit Arnd Bergmann
@ 2023-06-23 23:07 ` Kees Cook
0 siblings, 0 replies; 2+ messages in thread
From: Kees Cook @ 2023-06-23 23:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Johannes Berg, Arnd Bergmann, Christian Lamparter, Kalle Valo,
Johannes Berg, linux-wireless, linux-kernel, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Gregory Greenman,
Benjamin Berg, Ryder Lee, Ilan Peer, Felix Fietkau, Aloka Dixit,
Avraham Stern, Emmanuel Grumbach, netdev
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
^ permalink raw reply [flat|nested] 2+ messages in thread