[PATCH v1 0/2] wifi: mt76: Fix UAF and SKB leaks in RX A-MSDU path

[PATCH v1 0/2] wifi: mt76: Fix UAF and SKB leaks in RX A-MSDU path

[Eason Lai]

This patch series addresses a Use-After-Free bug and SKB memory leaks
 occurring in the RX A-MSDU reassembly codepath of the mt76 driver.

Eason Lai (2):
  wifi: mt76: flush a partial rx A-MSDU when its station is removed
  wifi: mt76: free a partial rx A-MSDU when the phy is unregistered

 drivers/net/wireless/mediatek/mt76/mac80211.c | 65 +++++++++++++++++++
 drivers/net/wireless/mediatek/mt76/mt76.h     |  1 +
 2 files changed, 66 insertions(+)

-- 
2.45.2

[PATCH v1 1/2] wifi: mt76: flush a partial rx A-MSDU when its station is removed

[Eason Lai]

mt76_rx_release_burst() keeps the first subframe of an A-MSDU in
phy->rx_amsdu[q] until the last subframe arrives, which can take several
polls. mt76_rx_status.wcid in that skb is only valid inside the RCU
section it was filled in.

If the station is removed meanwhile, the wcid is freed together with the
sta, and mt76_rx_convert() and mt76_check_ccmp_pn() use a dangling
pointer once the A-MSDU is released. Neither mt76_rx_aggr_stop() nor
__mt76_sta_remove() looks at rx_amsdu.

Drop a parked A-MSDU that references the wcid in mt76_wcid_cleanup(),
which every driver calls on station removal, and protect rx_amsdu with a
new per-queue lock.

Fixes: cc4b3c139ad3 ("mt76: mt7915: enable hw rx-amsdu de-aggregation")
Signed-off-by: Eason Lai <eason.lai@mediatek.com>
---
 drivers/net/wireless/mediatek/mt76/mac80211.c | 43 +++++++++++++++++++
 drivers/net/wireless/mediatek/mt76/mt76.h     |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index c9762082f473..19a2e8a26c3e 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -428,9 +428,12 @@ mt76_phy_init(struct mt76_phy *phy, struct ieee80211_hw *hw)
 {
 	struct mt76_dev *dev = phy->dev;
 	struct wiphy *wiphy = hw->wiphy;
+	int i;
 
 	INIT_LIST_HEAD(&phy->tx_list);
 	spin_lock_init(&phy->tx_lock);
+	for (i = 0; i < ARRAY_SIZE(phy->rx_amsdu); i++)
+		spin_lock_init(&phy->rx_amsdu[i].lock);
 	INIT_DELAYED_WORK(&phy->roc_work, mt76_roc_complete_work);
 	INIT_DELAYED_WORK(&phy->scan_work, mt76_scan_work);
 
@@ -974,10 +977,48 @@ void mt76_rx(struct mt76_dev *dev, enum mt76_rxq_id q, struct sk_buff *skb)
 	}
 #endif
 
+	spin_lock(&phy->rx_amsdu[q].lock);
 	mt76_rx_release_burst(phy, q, skb);
+	spin_unlock(&phy->rx_amsdu[q].lock);
 }
 EXPORT_SYMBOL_GPL(mt76_rx);
 
+/* Drop a partially received A-MSDU that still references the wcid of a
+ * station which is being removed.
+ */
+static void mt76_rx_amsdu_flush_wcid(struct mt76_dev *dev,
+				     struct mt76_wcid *wcid)
+{
+	int i, q;
+
+	for (i = 0; i < ARRAY_SIZE(dev->phys); i++) {
+		struct mt76_phy *phy = dev->phys[i];
+
+		if (!phy)
+			continue;
+
+		for (q = 0; q < ARRAY_SIZE(phy->rx_amsdu); q++) {
+			struct sk_buff *skb;
+			struct mt76_rx_status *status;
+
+			spin_lock_bh(&phy->rx_amsdu[q].lock);
+			skb = phy->rx_amsdu[q].head;
+			if (skb) {
+				status = (struct mt76_rx_status *)skb->cb;
+				if (status->wcid == wcid) {
+					phy->rx_amsdu[q].head = NULL;
+					phy->rx_amsdu[q].tail = NULL;
+				} else {
+					skb = NULL;
+				}
+			}
+			spin_unlock_bh(&phy->rx_amsdu[q].lock);
+
+			dev_kfree_skb(skb);
+		}
+	}
+}
+
 bool mt76_has_tx_pending(struct mt76_phy *phy)
 {
 	struct mt76_queue *q;
@@ -1760,6 +1801,8 @@ void mt76_wcid_cleanup(struct mt76_dev *dev, struct mt76_wcid *wcid)
 	struct sk_buff_head list;
 	struct sk_buff *skb;
 
+	mt76_rx_amsdu_flush_wcid(dev, wcid);
+
 	mt76_tx_status_lock(dev, &list);
 	mt76_tx_status_skb_get(dev, wcid, -1, &list);
 	idr_destroy(&wcid->pktid);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 9e851dea71d9..a4db4a2aaffa 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -913,6 +913,7 @@ struct mt76_phy {
 	u8 mac_work_count;
 
 	struct {
+		spinlock_t lock;
 		struct sk_buff *head;
 		struct sk_buff **tail;
 		u16 seqno;
-- 
2.45.2

[PATCH v1 2/2] wifi: mt76: free a partial rx A-MSDU when the phy is unregistered

[Eason Lai]

mt76_rx_release_burst() keeps the first subframes of an A-MSDU in
phy->rx_amsdu[q] until the last subframe arrives. If the phy or device
is unregistered in the meantime, nothing frees this skb and it leaks.

Free any parked A-MSDU after ieee80211_unregister_hw(), once rx has
stopped.

Fixes: cc4b3c139ad3 ("mt76: mt7915: enable hw rx-amsdu de-aggregation")
Signed-off-by: Eason Lai <eason.lai@mediatek.com>
---
 drivers/net/wireless/mediatek/mt76/mac80211.c | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index 19a2e8a26c3e..fb8494ad8f8d 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -603,6 +603,19 @@ int mt76_register_phy(struct mt76_phy *phy, bool vht,
 }
 EXPORT_SYMBOL_GPL(mt76_register_phy);
 
+static void mt76_rx_amsdu_purge(struct mt76_phy *phy)
+{
+	int q;
+
+	for (q = 0; q < ARRAY_SIZE(phy->rx_amsdu); q++) {
+		spin_lock_bh(&phy->rx_amsdu[q].lock);
+		dev_kfree_skb(phy->rx_amsdu[q].head);
+		phy->rx_amsdu[q].head = NULL;
+		phy->rx_amsdu[q].tail = NULL;
+		spin_unlock_bh(&phy->rx_amsdu[q].lock);
+	}
+}
+
 void mt76_unregister_phy(struct mt76_phy *phy)
 {
 	struct mt76_dev *dev = phy->dev;
@@ -614,6 +627,7 @@ void mt76_unregister_phy(struct mt76_phy *phy)
 		mt76_led_cleanup(phy);
 	mt76_tx_status_check(dev, true);
 	ieee80211_unregister_hw(phy->hw);
+	mt76_rx_amsdu_purge(phy);
 	dev->phys[phy->band_idx] = NULL;
 }
 EXPORT_SYMBOL_GPL(mt76_unregister_phy);
@@ -811,6 +825,7 @@ EXPORT_SYMBOL_GPL(mt76_register_device);
 void mt76_unregister_device(struct mt76_dev *dev)
 {
 	struct ieee80211_hw *hw = dev->hw;
+	int i;
 
 	if (!test_bit(MT76_STATE_REGISTERED, &dev->phy.state))
 		return;
@@ -820,6 +835,13 @@ void mt76_unregister_device(struct mt76_dev *dev)
 	mt76_tx_status_check(dev, true);
 	mt76_wcid_cleanup(dev, &dev->global_wcid);
 	ieee80211_unregister_hw(hw);
+
+	/* radio phys sharing dev->hw (e.g. mt7996) are not unregistered
+	 * separately
+	 */
+	for (i = 0; i < ARRAY_SIZE(dev->phys); i++)
+		if (dev->phys[i])
+			mt76_rx_amsdu_purge(dev->phys[i]);
 }
 EXPORT_SYMBOL_GPL(mt76_unregister_device);
 
-- 
2.45.2