Re: [PATCH v7 13/15] 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] Unsynchronized access and Time-of-Check to Time-of-Use (TOCTOU) race condition in the generic `qcom_pas` API.

Pre-existing issues:
- [High] Missing PAS shutdown and state cleanup on error and timeout paths in firmware power-up and power-down routines.
- [High] Repeated calls to devm_memremap() in ath12k_ahb_power_up() cause memory leaks across restart cycles.
--

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

wifi: ath12k: Switch to generic PAS TZ APIs

Switch ath12k client driver over to generic PAS TZ APIs. Generic PAS TZ
service allows to support multiple TZ implementation backends like QTEE
based SCM PAS service, OP-TEE based PAS service and any further future TZ
backend service.

> diff --git a/drivers/net/wireless/ath/ath12k/ahb.c b/drivers/net/wireless/ath/ath12k/ahb.c
> index 2dcf0a52e4c15..dff7fa7027c53 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 this function leak devres
structures and virtual address space on device restarts? 

The devm_memremap() function is called earlier in ath12k_ahb_power_up()
without a corresponding cleanup in ath12k_ahb_power_down(). Since
power_up/power_down can be called multiple times during driver recovery
(e.g., via ath12k_core_restart()), each power_up adds a new managed
resource, exhausting memory over time.

[Severity: High]
This is a pre-existing issue, but does this leave the PAS backend in an
inconsistent state if subsequent steps fail?

If qcom_pas_auth_and_reset() succeeds, but a later step like 
qcom_smem_state_update_bits() fails, the code jumps to err_fw2 without
calling qcom_pas_shutdown(). This leaves the peripheral running while
the host gives up, leading to resource leaks.

[Severity: High]
Does switching to the generic PAS API introduce a Time-of-Check to
Time-of-Use (TOCTOU) race condition?

The generic qcom_pas_auth_and_reset() API checks ops_ptr and
dereferences it without locks, RCU, or module references:

drivers/firmware/qcom/qcom_pas.c:qcom_pas_auth_and_reset() {
	if (!ops_ptr)
		return -ENODEV;

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

If the backend module is unloaded concurrently, setting ops_ptr to NULL
after the check but before the dereference, could this cause a
Use-After-Free or NULL pointer dereference?

>  		if (ret) {
>  			ath12k_err(ab, "failed to boot the remote processor %d\n", ret);
>  			goto err_fw2;
[ ... ]
> @@ -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);

[Severity: High]
This is a pre-existing issue, but does a timeout earlier in this
function leak resources?

If wait_for_completion_timeout() times out waiting for userpd_stopped, 
ath12k_ahb_power_down() returns immediately without clearing the stop
bit and without calling qcom_pas_shutdown(), abandoning the remote
processor state.

> +		ret = qcom_pas_shutdown(pasid);
>  		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/20260522115936.201208-1-sumit.garg@kernel.org?part=13