Received-SPF: softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <ad0756ac-ebaa-b5b3-6680-539e6986a11a@nbd.name>
Date:   Wed, 20 Apr 2022 12:40:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.8.0
Content-Language: en-US
To:     Bo Jiao <bo.jiao@mediatek.com>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>,
        Ryder Lee <ryder.lee@mediatek.com>,
        Sujuan Chen <sujuan.chen@mediatek.com>,
        Shayne Chen <shayne.chen@mediatek.com>,
        Evelyn Tsai <evelyn.tsai@mediatek.com>,
        linux-mediatek <linux-mediatek@lists.infradead.org>
References: <20220420031451.6770-1-bo.jiao@mediatek.com>
From:   Felix Fietkau <nbd@nbd.name>
Subject: Re: [PATCH] mt76: mt7915: fix msta->wcid use-after-free in
 mt76_tx_status_check()
In-Reply-To: <20220420031451.6770-1-bo.jiao@mediatek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);
  	}