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

[Baochen Qiang]

On 8/6/2026 3:21 AM, Julius Bairaktaris wrote:
> 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 <julius@xxxxxxxxxxxxxx>
> ---
>  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;
> +	}
> +

I understand this is a real issue but both changes look not a proper fix to me. at least
we should not test hal srng fields at CE layer since that is a layer violation.

>  	spin_lock_bh(&srng->lock);
> 
>  	ath11k_hal_srng_access_begin(ab, srng);