Re: [PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5379 bytes --]

On May 15, Cheng Hao Luo wrote:
> > struct ieee80211_tx_info {
> >         u32                        flags;                /*     0     4 */
> >         u32                        band:3;               /*     4: 0  4 */
> >         u32                        status_data_idr:1;    /*     4: 3  4 */
> >         u32                        status_data:13;       /*     4: 4  4 */
> >         u32                        hw_queue:4;           /*     4:17  4 */
> >         u32                        tx_time_est:10;       /*     4:21  4 */
> >
> >         /* XXX 1 bit hole, try to pack */
> >
> >         union {
> >                 struct {
> >                         union {
> >                                 struct {
> >                                         struct ieee80211_tx_rate rates[4]; /*     8    12 */
> >                                         s8     rts_cts_rate_idx; /*    20     1 */
> >                                         u8     use_rts:1; /*    21: 0  1 */
> >                                         u8     use_cts_prot:1; /*    21: 1  1 */
> >                                         u8     short_preamble:1; /*    21: 2  1 */
> >                                         u8     skip_table:1; /*    21: 3  1 */
> >                                         u8     antennas:2; /*    21: 4  1 */
> >                                 };                       /*     8    14 */
> >                                 long unsigned int jiffies; /*     8     8 */
> >                         };                               /*     8    16 */
> >                         struct ieee80211_vif * vif;      /*    24     8 */
> >                         struct ieee80211_key_conf * hw_key; /*    32     8 */
> >                         u32        flags;                /*    40     4 */
> >                         codel_time_t enqueue_time;       /*    44     4 */
> >                 } control;                               /*     8    40 */
> >                 struct {
> >                         u64        cookie;               /*     8     8 */
> >                 } ack;                                   /*     8     8 */
> >                 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 */
> >                         u8         pad;                  /*    27     1 */
> >                         u16        tx_time;              /*    28     2 */
> >                         u8         flags;                /*    30     1 */
> >                         u8         pad2;                 /*    31     1 */
> >                         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 */
> >
> >         /* size: 48, cachelines: 1, members: 7 */
> >         /* sum members: 44 */
> >         /* sum bitfield members: 31 bits, bit holes: 1, sum bit holes: 1 bits */
> >         /* last cacheline: 48 bytes */
> > };
> >
> > According to pahole, the size of the control inner union is actually 16 bytes
> > since the compiler adds 2 bytes of padding. Since mt76_tx_status_skb_add()
> > meset to 0 just mt76_tx_cb size (that is 16 bytes) I can't see how
> > control.flags is overwritten. Am I missing something?
> >
> > struct mt76_tx_cb {
> >         long unsigned int          jiffies;              /*     0     8 */
> >         u16                        wcid;                 /*     8     2 */
> >         u8                         pktid;                /*    10     1 */
> >         u8                         flags;                /*    11     1 */
> >
> >         /* size: 16, cachelines: 1, members: 4 */
> >         /* padding: 4 */
> >         /* last cacheline: 16 bytes */
> > };
> 
> Hi Lorenzo,
> 
> The mt76_tx_cb is placed at status.status_driver_data (offset 32).
> It overlaps with hw_key, flags and enqueue_time in the control union.
> 
> static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff *skb)
> {
> BUILD_BUG_ON(sizeof(struct mt76_tx_cb) >
>     sizeof(IEEE80211_SKB_CB(skb)->status.status_driver_data));
> return ((void *)IEEE80211_SKB_CB(skb)->status.status_driver_data);
> }

Hi Roy,

I still do not understand since mt76_tx_status_skb_add() sets to 0 just sizeof
of mt76_tx_cb, that according to pahole is 16 bytes, so it can't overwrite
hw_key pointer (whose offset respect to the beginning of the control struct is
24, 32 - 8).

Regards,
Lorenzo

> 
> Regards,
> Roy Luo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]