| Message ID | 20220420031451.6770-1-bo.jiao@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Felix Fietkau |
| Headers | show |
| Series | mt76: mt7915: fix msta->wcid use-after-free in mt76_tx_status_check() | expand |
On 20.04.22 05:14, Bo Jiao wrote: > From: Bo Jiao <Bo.Jiao@mediatek.com> > > fix msta->wcid use-after-free in mt76_tx_status_check when the sta > has been removed. > > Signed-off-by: Bo Jiao <Bo.Jiao@mediatek.com> > --- > drivers/net/wireless/mediatek/mt76/mt7915/main.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/main.c b/drivers/net/wireless/mediatek/mt76/mt7915/main.c > index 800f720..160d80e 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt7915/main.c > +++ b/drivers/net/wireless/mediatek/mt76/mt7915/main.c > @@ -701,6 +701,11 @@ void mt7915_mac_sta_remove(struct mt76_dev *mdev, struct ieee80211_vif *vif, > if (!list_empty(&msta->rc_list)) > list_del_init(&msta->rc_list); > spin_unlock_bh(&dev->sta_poll_lock); > + > + spin_lock_bh(&mdev->status_lock); > + if (!list_empty(&msta->wcid.list)) > + list_del_init(&msta->wcid.list); > + spin_unlock_bh(&mdev->status_lock); I'm trying to figure out where this use-after-free bug is coming from, and I can't seem to find the cause of it. Some context: mt7915_mac_sta_remove is called by __mt76_sta_remove, which also calls mt76_packet_id_flush afterwards. mt76_packet_id_flush calls mt76_tx_status_skb_get in a way that makes it iterate over all pending tx status packets and clearing them from the idr. If the idr is empty afterwards, it calls list_del_init(&wcid->list). The only way I can see your patch making a difference would be if clearing the idr fails. That could happen if for some unknown reason, cb->pktid is out of sync with the id that was used to add the packet to the idr. Can you please try the patch below and see if it avoids use-after-free issues and if it also shows the warning I added? Thanks, - Felix --- --- a/drivers/net/wireless/mediatek/mt76/tx.c +++ b/drivers/net/wireless/mediatek/mt76/tx.c @@ -181,7 +181,8 @@ mt76_tx_status_skb_get(struct mt76_dev *dev, struct mt76_wcid *wcid, int pktid, /* It has been too long since DMA_DONE, time out this packet * and stop waiting for TXS callback. */ - idr_remove(&wcid->pktid, cb->pktid); + WARN(id != cb->pktid, "Packet id %d does not match idr id %d\n", cb->pktid, id); + idr_remove(&wcid->pktid, id); __mt76_tx_status_skb_done(dev, skb, MT_TX_CB_TXS_FAILED | MT_TX_CB_TXS_DONE, list); }
diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/main.c b/drivers/net/wireless/mediatek/mt76/mt7915/main.c index 800f720..160d80e 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7915/main.c +++ b/drivers/net/wireless/mediatek/mt76/mt7915/main.c @@ -701,6 +701,11 @@ void mt7915_mac_sta_remove(struct mt76_dev *mdev, struct ieee80211_vif *vif, if (!list_empty(&msta->rc_list)) list_del_init(&msta->rc_list); spin_unlock_bh(&dev->sta_poll_lock); + + spin_lock_bh(&mdev->status_lock); + if (!list_empty(&msta->wcid.list)) + list_del_init(&msta->wcid.list); + spin_unlock_bh(&mdev->status_lock); } static void mt7915_tx(struct ieee80211_hw *hw,