Re: [PATCH v8 06/14] remoteproc: qcom_wcnss: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

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

New issues:
- [High] The generic PAS API functions in qcom_pas.c suffer from a TOCTOU race condition when reading the global ops_ptr, leading to a potential NULL pointer dereference.

Pre-existing issues:
- [High] Missing cleanup of SMD and Sysmon subdevices on wcnss_probe() error paths leads to resource leaks and Use-After-Free vulnerabilities.
- [High] Removing iris before deleting rproc in wcnss_remove() creates a Use-After-Free race condition.
- [High] Missing reinit_completion() causes spurious success on subsequent remoteproc starts or stops.
- [High] Infinite polling loop in wcnss_configure_iris() can cause kernel hangs.
--

commit f90353d8289b80221557c481e264c0861b3fde18
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 to use the generic PAS TZ
APIs. This allows support for multiple TZ implementations like QTEE or OP-TEE.

> 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);

[Severity: High]
This is a pre-existing issue, but can the infinite polling loops inside
wcnss_configure_iris() cause a hard kernel hang if the hardware is wedged?

Inside wcnss_configure_iris(), we poll hardware registers without a timeout:

drivers/remoteproc/qcom_wcnss.c:wcnss_configure_iris() {
    ...
    while (readl(wcnss->pmu_cfg) & WCNSS_PMU_IRIS_RESET_STS)
        cpu_relax();
    ...
    while (readl(wcnss->pmu_cfg) & WCNSS_PMU_IRIS_XO_CFG_STS)
        cpu_relax();
    ...
}

Could this lead to a denial of service if the register returns 0xffffffff or
remains unresponsive?

>  
> -	ret = qcom_scm_pas_auth_and_reset(WCNSS_PAS_ID);
> +	ret = qcom_pas_auth_and_reset(WCNSS_PAS_ID);

[Severity: High]
Does this new qcom_pas_auth_and_reset() call introduce a race condition?

Looking at the generic PAS API implementation in qcom_pas.c:

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);
}

Since ops_ptr is checked and then dereferenced without READ_ONCE() or locks,
could a concurrent backend module unload set ops_ptr to NULL between the check
and the dereference, causing a kernel panic?

[Severity: High]
This is a pre-existing issue, but does wcnss_start() risk spurious success
on subsequent starts if a previous wait timed out?

There is a call to wait_for_completion_timeout(&wcnss->start_done, ...) right
below this in wcnss_start(). If it times out, but a delayed ready interrupt
later calls complete(&wcnss->start_done), the next wcnss_start() could
instantly succeed without actually waiting for the firmware.

Should reinit_completion() be called before initiating the wait in both
wcnss_start() and wcnss_stop()?

>  	if (ret) {
>  		dev_err(wcnss->dev,
>  			"failed to authenticate image and release reset\n");

[ ... ]

> @@ -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;

[Severity: High]
This is a pre-existing issue, but do the error paths in wcnss_probe() leak
the SMD and Sysmon subdevices?

In wcnss_probe(), qcom_add_sysmon_subdev() allocates memory and registers a
notifier to a global chain. If a subsequent step like qcom_iris_probe()
fails, the error path jumps to remove_iris or detach_pds:

drivers/remoteproc/qcom_wcnss.c:wcnss_probe() {
    ...
remove_iris:
    qcom_iris_remove(wcnss->iris);
detach_pds:
    wcnss_release_pds(wcnss);

    return ret;
}

This path fails to call qcom_remove_sysmon_subdev() and
qcom_remove_smd_subdev(). Could this leave the sysmon object in the global
notifier chain, leading to a use-after-free when the notifier triggers?

[Severity: High]
This is a pre-existing issue, but does wcnss_remove() have a use-after-free
race condition by freeing the iris object before stopping the remoteproc?

drivers/remoteproc/qcom_wcnss.c:wcnss_remove() {
    ...
    qcom_iris_remove(wcnss->iris);
    rproc_del(wcnss->rproc);
    ...
}

Because rproc_del() hasn't stopped the remoteproc yet, userspace could
concurrently trigger wcnss_start(). Since wcnss_start() checks
if (!wcnss->iris), which passes because the pointer is freed but not nulled,
it then calls qcom_iris_enable(wcnss->iris) on freed memory.

Should rproc_del() be called before qcom_iris_remove() to prevent this?

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