2022-04-21 16:39:19

by Bo Jiao

Subject: [PATCH] mt76: mt7915: fix msta->wcid use-after-free in mt76_tx_status_check()

From: Bo Jiao <[email protected]>

fix msta->wcid use-after-free in mt76_tx_status_check when the sta
has been removed.

Signed-off-by: Bo Jiao <[email protected]>
---
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);
}

static void mt7915_tx(struct ieee80211_hw *hw,
--
2.18.0

2022-04-22 21:59:48

by Felix Fietkau

Subject: Re: [PATCH] mt76: mt7915: fix msta->wcid use-after-free in mt76_tx_status_check()

On 20.04.22 05:14, Bo Jiao wrote:
> From: Bo Jiao <[email protected]>
>
> fix msta->wcid use-after-free in mt76_tx_status_check when the sta
> has been removed.
>
> Signed-off-by: Bo Jiao <[email protected]>
> ---
> 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);
}