[PATCH] wifi: mt76: mt7996: avoid memset overwriting tx_info->control.flags
Ryder Lee
Ryder.Lee at mediatek.com
Tue May 19 13:42:39 PDT 2026
On Tue, 2026-05-19 at 14:24 +0200, Lorenzo Bianconi wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> >
> > On Mon, May 18, 2026 at 5:31 AM Lorenzo Bianconi
> > <lorenzo at kernel.org> wrote:
> > >
> > > 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
> >
> > Hi Lorenzo,
> >
> > The mt76_tx_status_skb_add() memset zero the 16 bytes starting from
> > status.status_driver_data (please see the above inline function
> > shared
> > in my last response) whose offset with respect to the beginning of
> > the control/status union is exactly 24 (32 - 8) instead of 0.
> >
> > Regards,
> > Roy Luo
>
> Hi Roy,
>
> I can see the issue now, I was confusing status.status_driver_data
> with
> driver_data. You are right, we have an issue here. However, copying
> all the
> ieee80211_tx_info struct seems a bit overkill, what do you think?
> Moreover, we have the same issue for various chipsets (e.g. mt7925
> and
> mt7915). I guess we should try to find a global solution for the
> problem.
>
> Regards,
> Lorenzo
What about adding an helper for cb operation?
+void
+mt76_tx_status_skb_cb_add(struct mt76_dev *dev, struct sk_buff *skb,
+ struct mt76_wcid *wcid, int pid)
+{
+ struct mt76_tx_cb *cb = mt76_tx_skb_cb(skb);
+
+ memset(cb, 0, sizeof(*cb));
+
+ spin_lock_bh(&dev->status_lock);
+ cb->wcid = wcid->idx;
+ cb->pktid = pid;
+ spin_unlock_bh(&dev->status_lock);
+}
+EXPORT_SYMBOL_GPL(mt76_tx_status_skb_cb_add);
And add this for each chipset.
index 061ab66..d0b67a2 100644
--- a/mt7996/mac.c
+++ b/mt7996/mac.c
@@ -1108,6 +1108,7 @@ int mt7996_tx_prepare_skb(struct mt76_dev *mdev,
void *txwi_ptr,
if (!is_8023 || pid >= MT_PACKET_ID_FIRST)
mt7996_mac_write_txwi(dev, txwi_ptr, tx_info->skb,
wcid, key,
pid, qid, 0);
+ mt76_tx_status_skb_cb_add(dev, tx_info->skb, wcid, pid);
More information about the Linux-mediatek
mailing list