Re: [PATCH v9 06/14] remoteproc: qcom_wcnss: 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] The generic qcom_pas API lacks synchronization against concurrent registration and backend unregistration, leading to TOCTOU races and Use-After-Free.
- [High] Blindly trusting param[1].u.memref.size from the second TEE invocation in qcom_pas_tee_get_rsc_table() leads to an out-of-bounds read and kernel memory leak.

Pre-existing issues:
- [High] The wcnss_probe() function fails to clean up sysmon and smd subdevices on error paths, leading to memory/socket leaks and dangling pointers in a global notifier chain.
--

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

remoteproc: qcom_wcnss: Switch to generic PAS TZ APIs

Switch qcom_wcnss 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.

> Switch qcom_wcnss 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.

[Severity: High]
Is there an out-of-bounds read and kernel memory leak in the OP-TEE PAS
service?

In drivers/firmware/qcom/qcom_pas_tee.c:qcom_pas_tee_get_rsc_table(), a
shared memory buffer is allocated based on the size from the first TEE
invocation. In the second TEE invocation, the TEE can modify
param[1].u.memref.size.

The function then blindly trusts this size:

    *output_rt_size = param[1].u.memref.size;
    rt_buf = kmemdup(rt_shm_va, *output_rt_size, GFP_KERNEL);

Can a compromised TrustZone environment return an increased size, causing an
out-of-bounds read from the originally allocated buffer?

> 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: High]
Can this introduce a TOCTOU race window?

If multiple PAS backends probe concurrently, qcom_pas_ops_register() in
drivers/firmware/qcom/qcom_pas.c performs a lockless read-modify-write of
ops_ptr:

    if (!qcom_pas_is_available())
        smp_store_release(&ops_ptr, ops);

This allows one backend to silently overwrite another.

Additionally, when this driver calls qcom_pas_auth_and_reset(), it lacks
RCU or try_module_get() protection:

    if (!ops_ptr)
        return -ENODEV;

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

If the backend module is unloaded concurrently, ops_ptr is unregistered and
the module memory is freed, leaving this driver to dereference a dangling
function pointer. Is there a way to serialize these registrations and
protect the backend access?

[ ... ]
> @@ -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 does wcnss_probe() fail to clean up
subdevices on error paths?

Earlier in wcnss_probe(), qcom_add_sysmon_subdev() and qcom_add_smd_subdev()
are called. If qcom_iris_probe() subsequently fails (e.g., returning
-EPROBE_DEFER because regulators are not ready), the error path jumps to
detach_pds:

    if (IS_ERR(wcnss->iris)) {
        ret = PTR_ERR(wcnss->iris);
        goto detach_pds;
    }

And the cleanup path skips subdevice removal entirely:

    remove_iris:
        qcom_iris_remove(wcnss->iris);
    detach_pds:
        wcnss_release_pds(wcnss);

Since qcom_add_sysmon_subdev() allocates memory, creates a QMI socket, and
registers a notifier on the global sysmon_notifiers chain, does this leak
these resources on every probe deferral?

Furthermore, sysmon->dev caches the platform device pointer, which becomes
a dangling pointer if the device is ultimately removed. Could this lead to
a use-after-free when sysmon_notify iterates over the leaked notifiers?

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