[PATCH ath-next 0/2] wifi: ath11k/ath12k: fix a double free when the first fragment's descriptor copy fails

[PATCH ath-next 0/2] wifi: ath11k/ath12k: fix a double free when the first fragment's descriptor copy fails

[Adriel Santos]

In both drivers, the RX defragmentation path queues fragment 0 before it
copies the fragment's REO destination descriptor. If that GFP_ATOMIC copy
fails, the caller frees the skb while it is still queued, which leads to a
double free or a use after free plus a NULL dereference on reinjection.
Both patches move the copy before the queueing.

Patch 1 was tested on IPQ5018 on an OpenWrt backports tree, with forced
allocation failures (details in its notes); patch 2 is compile tested
only.

Both bugs were found during an LLM-assisted review of the RX fragment
handling, and an LLM coding assistant also helped write the patches and
run the bench tests described in patch 1's notes. I reviewed the
analysis, the code and the test results, and I am responsible for both
patches.

Adriel Santos (2):
  wifi: ath11k: copy the first fragment's descriptor before queuing it
  wifi: ath12k: copy the first fragment's descriptor before queuing it

 drivers/net/wireless/ath/ath11k/dp_rx.c       | 22 ++++++++++---------
 drivers/net/wireless/ath/ath12k/wifi7/dp_rx.c | 22 ++++++++++---------
 2 files changed, 24 insertions(+), 20 deletions(-)

-- 
2.47.3

[PATCH ath-next 1/2] wifi: ath11k: copy the first fragment's descriptor before queuing it

[Adriel Santos]

ath11k_dp_rx_frag_h_mpdu() queues a received fragment on rx_tid->rx_frags
and sets its bit in rx_frag_bitmap before, for fragment 0, it copies the
REO destination descriptor with kmemdup(GFP_ATOMIC). If that copy fails,
the function returns -ENOMEM and its caller,
ath11k_dp_process_rx_err_buf(), frees the msdu and returns its link
descriptor while the msdu is still on rx_frags:

- the next flush of rx_frags (a new sequence number, the fragment timer,
  peer teardown or a key install) frees the skb a second time;
- if the remaining fragments of the MPDU arrive first, the bitmap is
  complete, ath11k_dp_rx_h_defrag() walks the list with the freed skb,
  and ath11k_dp_rx_h_defrag_reo_reinject() dereferences the NULL
  rx_tid->dst_ring_desc.

This has not been seen in the field; it was reproduced only by forcing
the allocation to fail.

Copy the descriptor before the fragment is queued, so a failure leaves
nothing of this fragment in rx_tid and the caller's free is the only
one. Fragments other than 0 still return their link descriptor after
being queued.

Found during an LLM-assisted review of the RX defragmentation path; the
fix and its testing were LLM-assisted as well.

Tested-on: IPQ5018 hw1.0 AHB WLAN.HK.2.7.0.1-01744-QCAHKSWPL_SILICONZ-1

Fixes: 243874c64c81 ("ath11k: handle RX fragments")
Signed-off-by: Adriel Santos <adriel@adriel.eu>
---

Notes:
    How this was tested, and what was not done:
    
    - Found while debugging fragment reception on an IPQ5018 board; the
      failure needs a 64-byte GFP_ATOMIC allocation to fail, so it was never
      seen in normal use. Under Documentation/process/threat-model.rst it is
      a regular bug.
    - To exercise the error path, a test-only module parameter made the
      kmemdup() fail for every Nth first fragment (N = 1 or 3), with a
      client sending 1000-byte pings and TCP uploads fragmented at 256 bytes
      on 2.4 GHz (host data path), over two days:
      - without this patch, the board crashed and rebooted within seconds,
        4 times out of 4; in the two runs that sent one ping at a time, at
        the first forced failure. There is no serial console, so no oops
        trace was captured.
      - with it, 9365 forced failures over 22 minutes: each affected frame
        was dropped, with no warning, SUnreclaim flat, and normal reception
        once the parameter was cleared.
    - The tests ran on OpenWrt's backports of this driver (with downstream
      patches; NSS offload disabled for the radio), not on ath-next. The
      function and its caller are the same there; on ath-next the patch is
      build-tested only (arm64, W=1, no new warnings).
    - That tree also carries a not-yet-submitted change that programs the
      IPQ5018 RX fragment destination in REO1_MISC_CTL, which ath-next does
      not write for this chip. Without it, fragments may not reach this
      function on IPQ5018 at all, so ath-next may not take the tested path
      on that chip.
    - ath12k has the same order in ath12k/wifi7/dp_rx.c; no ath12k hardware
      was available to test a fix.

 drivers/net/wireless/ath/ath11k/dp_rx.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/dp_rx.c b/drivers/net/wireless/ath/ath11k/dp_rx.c
index 6d95f7b34..789a56488 100644
--- a/drivers/net/wireless/ath/ath11k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath11k/dp_rx.c
@@ -3674,6 +3674,17 @@ static int ath11k_dp_rx_frag_h_mpdu(struct ath11k *ar,
 		goto out_unlock;
 	}
 
+	/* On failure the caller frees msdu, so it must not be queued yet */
+	if (frag_no == 0) {
+		rx_tid->dst_ring_desc = kmemdup(ring_desc,
+						sizeof(*rx_tid->dst_ring_desc),
+						GFP_ATOMIC);
+		if (!rx_tid->dst_ring_desc) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+	}
+
 	if (!rx_tid->rx_frag_bitmap || (frag_no > __fls(rx_tid->rx_frag_bitmap)))
 		__skb_queue_tail(&rx_tid->rx_frags, msdu);
 	else
@@ -3683,18 +3694,9 @@ static int ath11k_dp_rx_frag_h_mpdu(struct ath11k *ar,
 	if (!more_frags)
 		rx_tid->last_frag_no = frag_no;
 
-	if (frag_no == 0) {
-		rx_tid->dst_ring_desc = kmemdup(ring_desc,
-						sizeof(*rx_tid->dst_ring_desc),
-						GFP_ATOMIC);
-		if (!rx_tid->dst_ring_desc) {
-			ret = -ENOMEM;
-			goto out_unlock;
-		}
-	} else {
+	if (frag_no != 0)
 		ath11k_dp_rx_link_desc_return(ab, ring_desc,
 					      HAL_WBM_REL_BM_ACT_PUT_IN_IDLE);
-	}
 
 	if (!rx_tid->last_frag_no ||
 	    rx_tid->rx_frag_bitmap != GENMASK(rx_tid->last_frag_no, 0)) {
-- 
2.47.3

[PATCH ath-next 2/2] wifi: ath12k: copy the first fragment's descriptor before queuing it

[Adriel Santos]

ath12k_wifi7_dp_rx_frag_h_mpdu() queues a received fragment on
rx_tid->rx_frags and sets its bit in rx_frag_bitmap before, for fragment
0, it copies the REO destination descriptor with kmemdup(GFP_ATOMIC). If
that copy fails, the function returns -ENOMEM and its caller,
ath12k_wifi7_dp_process_rx_err_buf(), frees the msdu and returns its link
descriptor while the msdu is still on rx_frags:

- the next flush of rx_frags (a new sequence number, the fragment timer
  or peer teardown) frees the skb a second time;
- if the remaining fragments of the MPDU arrive first, the bitmap is
  complete, ath12k_wifi7_dp_rx_h_defrag() walks the list with the freed
  skb, and ath12k_wifi7_dp_rx_h_defrag_reo_reinject() dereferences the
  NULL rx_tid->dst_ring_desc.

This has not been seen in the field; it was reproduced, on ath11k only,
by forcing the allocation to fail.

Copy the descriptor before the fragment is queued, so a failure leaves
nothing of this fragment in rx_tid and the caller's free is the only
one. Fragments other than 0 still return their link descriptor after
being queued.

ath11k has the same code and gets the same fix.

Found during an LLM-assisted review of the RX defragmentation path; the
fix was LLM-assisted as well.

Compile tested only.

Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Signed-off-by: Adriel Santos <adriel@adriel.eu>
---

Notes:
    Found while fixing the same bug in ath11k (previous patch), which was
    tested on IPQ5018 with forced kmemdup() failures. No ath12k hardware was
    available: this patch is build-tested only (arm64, W=1, no new
    warnings), and no reproducer was run on ath12k.

 drivers/net/wireless/ath/ath12k/wifi7/dp_rx.c | 22 ++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/wifi7/dp_rx.c b/drivers/net/wireless/ath/ath12k/wifi7/dp_rx.c
index a32c80440..d84cbb8df 100644
--- a/drivers/net/wireless/ath/ath12k/wifi7/dp_rx.c
+++ b/drivers/net/wireless/ath/ath12k/wifi7/dp_rx.c
@@ -1227,6 +1227,17 @@ static int ath12k_wifi7_dp_rx_frag_h_mpdu(struct ath12k_pdev_dp *dp_pdev,
 		goto out_unlock;
 	}
 
+	/* On failure the caller frees msdu, so it must not be queued yet */
+	if (frag_no == 0) {
+		rx_tid->dst_ring_desc = kmemdup(ring_desc,
+						sizeof(*rx_tid->dst_ring_desc),
+						GFP_ATOMIC);
+		if (!rx_tid->dst_ring_desc) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+	}
+
 	if ((!rx_tid->rx_frag_bitmap || frag_no > __fls(rx_tid->rx_frag_bitmap)))
 		__skb_queue_tail(&rx_tid->rx_frags, msdu);
 	else
@@ -1236,18 +1247,9 @@ static int ath12k_wifi7_dp_rx_frag_h_mpdu(struct ath12k_pdev_dp *dp_pdev,
 	if (!more_frags)
 		rx_tid->last_frag_no = frag_no;
 
-	if (frag_no == 0) {
-		rx_tid->dst_ring_desc = kmemdup(ring_desc,
-						sizeof(*rx_tid->dst_ring_desc),
-						GFP_ATOMIC);
-		if (!rx_tid->dst_ring_desc) {
-			ret = -ENOMEM;
-			goto out_unlock;
-		}
-	} else {
+	if (frag_no != 0)
 		ath12k_wifi7_dp_rx_link_desc_return(dp, &ring_desc->buf_addr_info,
 						    HAL_WBM_REL_BM_ACT_PUT_IN_IDLE);
-	}
 
 	if (!rx_tid->last_frag_no ||
 	    rx_tid->rx_frag_bitmap != GENMASK(rx_tid->last_frag_no, 0)) {
-- 
2.47.3