Re: [PATCH v9 12/14] wifi: ath12k: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Lockless double-read of global ops_ptr causes TOCTOU NULL pointer dereference.

Pre-existing issues:
- [High] Repeated calls to devm_memremap() cause memory/vmap leaks during firmware recovery.
- [High] Missing qcom_pas_shutdown() on error paths leaves remote processor running.
--

commit e338489a518c55193a64126b6633cfb19d87e0ba
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

wifi: ath12k: Switch to generic PAS TZ APIs

This commit switches the ath12k client driver over to generic PAS TZ APIs
to support multiple TZ implementation backends.

> diff --git a/drivers/net/wireless/ath/ath12k/ahb.c b/drivers/net/wireless/ath/ath12k/ahb.c
> index 30733a244454e..69e21214e6290 100644
> --- a/drivers/net/wireless/ath/ath12k/ahb.c
> +++ b/drivers/net/wireless/ath/ath12k/ahb.c
[ ... ]
> @@ -420,7 +420,7 @@ static int ath12k_ahb_power_up(struct ath12k_base *ab)
>  
>  	if (ab_ahb->scm_auth_enabled) {
>  		/* Authenticate FW image using peripheral ID */
> -		ret = qcom_scm_pas_auth_and_reset(pasid);
> +		ret = qcom_pas_auth_and_reset(pasid);

[Severity: High]
This is a pre-existing issue, but does ath12k_ahb_power_up() leak memory
during firmware recovery?

Earlier in this function, devm_memremap() is used:

    mem_region = devm_memremap(dev, mem_phys, mem_size, MEMREMAP_WC);

When the ath12k device undergoes a firmware crash recovery or reset,
ath12k_core_reset() calls ath12k_hif_power_up(), which calls
ath12k_ahb_power_up(). Because devm_memremap() allocates device-managed memory,
and the device itself is not destroyed or unbound during a reset, the memory
mapping is permanently leaked into the device's resource list.

[Severity: High]
Does calling this new API introduce a TOCTOU NULL pointer dereference?

Looking at qcom_pas_auth_and_reset() in drivers/firmware/qcom/qcom_pas.c:

int qcom_pas_auth_and_reset(u32 pas_id)
{
	if (!ops_ptr)
		return -ENODEV;

	return ops_ptr->auth_and_reset(ops_ptr->dev, pas_id);
}

If a concurrent module unload of the PAS backend occurs, ops_ptr could be set
to NULL between the check and the dereference. Even with READ_ONCE(), the lack
of module reference counting allows the backend to be unloaded while its
functions are executing. The same issue exists in qcom_pas_shutdown().

>  		if (ret) {
>  			ath12k_err(ab, "failed to boot the remote processor %d\n", ret);
>  			goto err_fw2;

[Severity: High]
This is a pre-existing issue, but does this error path leave the remote
processor running?

If qcom_pas_auth_and_reset() succeeds, but a subsequent step like updating
the SMEM state or waiting for completion fails, the code jumps to err_fw2 and
returns an error without shutting down the PAS. This can lead to memory
corruption if the driver unloads and the remote processor continues to DMA
into freed kernel memory.

[ ... ]
> @@ -485,10 +485,10 @@ static void ath12k_ahb_power_down(struct ath12k_base *ab, bool is_suspend)
>  		pasid = (u32_encode_bits(ab_ahb->userpd_id, ATH12K_USERPD_ID_MASK)) |
>  			 ATH12K_AHB_UPD_SWID;
>  		/* Release the firmware */
> -		ret = qcom_scm_pas_shutdown(pasid);
> +		ret = qcom_pas_shutdown(pasid);

[Severity: High]
This is also a pre-existing issue, but does ath12k_ahb_power_down() skip
shutting down the PAS on timeout?

If the wait for userpd_stopped times out earlier in this function:

    if (!time_left) {
        ath12k_err(ab, "UserPD stop wait timed out\n");
        return;
    }

The function returns early without invoking qcom_pas_shutdown(), which can
also leave the remote processor running.

>  		if (ret)
> -			ath12k_err(ab, "scm pas shutdown failed for userPD%d\n",
> -				   ab_ahb->userpd_id);
> +			ath12k_err(ab, "PAS shutdown failed for userPD%d: %d\n",
> +				   ab_ahb->userpd_id, ret);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702115835.167602-1-sumit.garg@kernel.org?part=12