Re: [PATCH v7 07/15] remoteproc: qcom_wcnss: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unprotected TOCTOU access to global `ops_ptr` in generic PAS APIs leads to NULL pointer dereferences and Use-After-Free.

Pre-existing issues:
- [High] Completions for start and stop are never reinitialized, allowing timeout bypasses on subsequent invocations.
- [High] Missing cleanup for subdevices in probe error paths causes memory leaks and Use-After-Free.
- [High] Teardown order race between remote processor deletion and `qcom_iris_remove` leads to Use-After-Free.
--

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

remoteproc: qcom_wcnss: Switch to generic PAS TZ APIs
    
This commit switches the qcom_wcnss client driver over to use the generic PAS
TZ APIs instead of the qcom_scm APIs directly. This change allows the driver
to support multiple TrustZone implementation backends.

> diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
> index 4add9037dbd5a..0dbdd18ab3dd9 100644
> --- a/drivers/remoteproc/qcom_wcnss.c
> +++ b/drivers/remoteproc/qcom_wcnss.c
[ ... ]
> @@ -257,7 +257,7 @@ static int wcnss_start(struct rproc *rproc)
>  	wcnss_indicate_nv_download(wcnss);
>  	wcnss_configure_iris(wcnss);
>  
> -	ret = qcom_scm_pas_auth_and_reset(WCNSS_PAS_ID);
> +	ret = qcom_pas_auth_and_reset(WCNSS_PAS_ID);

[Severity: Critical]
Does this new generic API introduce an unprotected time-of-check to
time-of-use race condition?

Looking at the generic PAS framework, the global ops_ptr is checked for
NULL and subsequently dereferenced without locking or module reference
counting.

If the PAS provider module is unbound or unloaded concurrently, could
ops_ptr become NULL between the check and the dereference?

Furthermore, even if ops_ptr is not NULL during the check, could the
provider module be unloaded, causing ops_ptr->supported to point to
unmapped memory and ops_ptr->dev to be freed?

Could this lead to a use-after-free crash?

>  	if (ret) {
>  		dev_err(wcnss->dev,
>  			"failed to authenticate image and release reset\n");
> @@ -269,7 +269,7 @@ static int wcnss_start(struct rproc *rproc)
>  	if (wcnss->ready_irq > 0 && ret == 0) {
>  		/* We have a ready_irq, but it didn't fire in time. */
>  		dev_err(wcnss->dev, "start timed out\n");
> -		qcom_scm_pas_shutdown(WCNSS_PAS_ID);
> +		qcom_pas_shutdown(WCNSS_PAS_ID);

[Severity: High]
This is a pre-existing issue, but does this timeout path properly reset the
completion for future invocations?

In wcnss_start() and wcnss_stop(), the code waits for the start_done and
stop_done completions using wait_for_completion_timeout().

If a timeout occurs, the completion's internal counter is not decremented.
If the hardware later triggers the interrupt, complete() increments the
counter.

Since reinit_completion() is never called before initiating a new operation,
will subsequent calls to wait_for_completion_timeout() return immediately
instead of waiting for the hardware, bypassing synchronization entirely?

>  		ret = -ETIMEDOUT;
>  		goto disable_iris;
>  	}
[ ... ]
> @@ -557,10 +557,10 @@ static int wcnss_probe(struct platform_device *pdev)
>  
>  	data = of_device_get_match_data(&pdev->dev);
>  
> -	if (!qcom_scm_is_available())
> +	if (!qcom_pas_is_available())
>  		return -EPROBE_DEFER;
>  
> -	if (!qcom_scm_pas_supported(WCNSS_PAS_ID)) {
> +	if (!qcom_pas_supported(WCNSS_PAS_ID)) {

[Severity: High]
This is a pre-existing issue, but does wcnss_probe() fail to clean up
subdevices in its error paths?

During probe, qcom_add_sysmon_subdev() and qcom_add_smd_subdev() are called.
The sysmon subdevice allocates memory and registers global notifiers
referencing the remoteproc.

If a subsequent step in probe fails, the error path jumps to detach_pds, which
skips calling qcom_remove_sysmon_subdev() and qcom_remove_smd_subdev().

The parent rproc object is automatically freed by devm_rproc_alloc cleanup.

Does this leave the global sysmon notifier with a dangling pointer that will
cause a use-after-free when the notifier later fires?


[Severity: High]
This is another pre-existing issue, but is there a teardown order race
between remote processor deletion and qcom_iris_remove() that can lead
to a use-after-free?

In wcnss_remove(), qcom_iris_remove() is called before rproc_del().

rproc_del() halts the remote processor and prevents new start invocations.
Because the iris object is removed first, could a concurrent sysfs operation
invoke wcnss_start()?

wcnss_start() takes iris_lock and checks if (!wcnss->iris), but wcnss->iris
is never set to NULL after allocation in probe.

Will wcnss_start() proceed to use the removed and freed wcnss->iris object?

Should rproc_del() be called first to serialize this correctly?

>  		dev_err(&pdev->dev, "PAS is not available for WCNSS\n");
>  		return -ENXIO;
>  	}

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