[PATCH ath-next 1/2] wifi: ath11k: guard copy engine access against a cleared srng

Julius Bairaktaris <[email protected]>
Newsgroups org.infradead.lists.ath11k,org.kernel.vger.linux-kernel,org.kernel.vger.linux-wireless
Message-ID <[email protected]>
ath11k_core_reconfigure_on_crash() zeroes the ring list with
ath11k_hal_srng_clear() and only rebuilds the copy engine rings later,
inside ath11k_core_qmi_firmware_ready() -> ath11k_ce_init_pipes().

ath11k_ce_send() is gated on ATH11K_FLAG_CRASH_FLUSH, but that flag is
cleared before ath11k_core_qmi_firmware_ready() is called - it has to be,
since the firmware bring-up itself sends WMI commands through the copy
engine. For the whole of that bring-up the gate is open while the ring is
still zeroed, so any WMI command issued in that window dereferences the
NULL u.src_ring.tp_addr in ath11k_hal_srng_access_begin():

  Unable to handle kernel read from unreadable memory at virtual address 0
  CPU: 0 UID: 101 PID: 1846 Comm: hostapd
  pc : ath11k_hal_srng_access_begin+0xc/0x60 [ath11k]
  lr : ath11k_ce_send+0x114/0x350 [ath11k]
  Call trace:
   ath11k_hal_srng_access_begin+0xc/0x60 [ath11k] (P)
   ath11k_htc_send+0x188/0x3c8 [ath11k]
   ath11k_wmi_cmd_send+0xc8/0x2f4 [ath11k]
   ath11k_wmi_send_peer_create_cmd+0x78/0xe0 [ath11k]
   ath11k_peer_create+0x138/0x500 [ath11k]

The caller matters: this is a command issued fresh during the recovery,
not a task that was already blocked in ath11k_wmi_cmd_send(). A blocked
one sleeps through the whole window and never re-enters ath11k_ce_send(),
which is why hammering beacon updates does not reproduce it and a station
associating during recovery does.

ath11k_ce_completed_send_next() has no gate at all, and
ath11k_core_reconfigure_on_crash() calls ath11k_ce_cleanup_pipes() before
anything else, so a crash arriving after a previous recovery already ran
ath11k_hal_srng_clear() reaps rings whose tp_addr is NULL. This is a
different entry into the same dereference from the one fixed by
commit e8d85672dd7e ("wifi: ath11k: fix NULL pointer dereference in
ath11k_hal_srng_access_begin"), which stops a successful bring-up from
being repeated; it does not close this window.

Test u.src_ring.tp_addr rather than srng->initialized in both places:
ath11k_hal_srng_setup() publishes ->initialized before ->tp_addr with no
barrier between them, so an ->initialized check can be true while the
pointer that is about to be dereferenced is still NULL.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.12-01460-QCAHKSWPL_SILICONZ-1

Fixes: 32be3ca4cf78 ("wifi: ath11k: HAL SRNG: don't deinitialize and re-initialize again")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <[email protected]>
---
 drivers/net/wireless/ath/ath11k/ce.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -472,6 +472,17 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe

 	spin_lock_bh(&srng->lock);

+	/*
+	 * ath11k_ce_cleanup_pipes() can reach this on a ring that
+	 * ath11k_hal_srng_clear() has zeroed and that recovery has not
+	 * rebuilt. Bail out as for an empty ring rather than dereference
+	 * the NULL tp_addr in ath11k_hal_srng_access_begin().
+	 */
+	if (unlikely(!srng->u.src_ring.tp_addr)) {
+		skb = ERR_PTR(-EIO);
+		goto err_unlock;
+	}
+
 	ath11k_hal_srng_access_begin(ab, srng);

 	desc = ath11k_hal_srng_src_reap_next(ab, srng);
@@ -750,6 +761,19 @@ int ath11k_ce_send(struct ath11k_base *ab, struct sk_buff *skb, u8 pipe_id,

 	srng = &ab->hal.srng_list[pipe->src_ring->hal_ring_id];

+	/*
+	 * ATH11K_FLAG_CRASH_FLUSH is cleared before the copy engine rings
+	 * are rebuilt, so the check above lets a send through while the
+	 * ring is still zeroed. Test the pointer that would be
+	 * dereferenced: ath11k_hal_srng_setup() publishes ->initialized
+	 * before ->tp_addr without a barrier, so ->initialized would still
+	 * race the rebuild.
+	 */
+	if (unlikely(!srng->u.src_ring.tp_addr)) {
+		spin_unlock_bh(&ab->ce.ce_lock);
+		return -ESHUTDOWN;
+	}
+
 	spin_lock_bh(&srng->lock);

 	ath11k_hal_srng_access_begin(ab, srng);
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.