Re: [PATCH v6 02/16] firmware: qcom: Add a generic PAS service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] TOCTOU race condition and missing memory barriers on global `ops_ptr` accesses
- [High] Race condition in `qcom_pas_ops_register()` allows silent overwrites and NULL pointer dereference
- [High] Use-After-Free vulnerability due to missing module lifetime synchronization
--

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

firmware: qcom: Add a generic PAS service

This commit introduces a generic Peripheral Authentication Service layer for
Qualcomm platforms. It acts as a common interface for different TrustZone
backends to support both SCM and TEE bus-based PAS services.

Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
Tested-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> # Lemans
Reviewed-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>

> diff --git a/drivers/firmware/qcom/qcom_pas.c b/drivers/firmware/qcom/qcom_pas.c
> new file mode 100644
> index 0000000000000..bc6c42f2b3c61
> --- /dev/null
> +++ b/drivers/firmware/qcom/qcom_pas.c
[ ... ]
> +int qcom_pas_init_image(u32 pas_id, const void *metadata, size_t size,
> +			struct qcom_pas_context *ctx)
> +{
> +	if (!ops_ptr)
> +		return -ENODEV;
> +
> +	return ops_ptr->init_image(ops_ptr->dev, pas_id, metadata, size, ctx);
> +}

Could a race condition occur here if qcom_pas_ops_unregister() is called
concurrently? The global ops_ptr is read twice via plain C reads. If it
becomes NULL after the check, the second read might cause a NULL pointer
dereference.

Also, since there is no smp_load_acquire() or READ_ONCE() used to cache the
pointer locally, could weakly ordered architectures observe a non-NULL
ops_ptr but read uninitialized callback pointers?

[ ... ]
> +void qcom_pas_ops_register(struct qcom_pas_ops *ops)
> +{
> +	if (!qcom_pas_is_available())
> +		/* Paired with smp_load_acquire() in qcom_pas_is_available() */
> +		smp_store_release(&ops_ptr, ops);
> +	else
> +		pr_err("qcom_pas: ops already registered by %s\n",
> +		       ops_ptr->drv_name);
> +}

What happens if two backend drivers attempt to register concurrently?
The check in qcom_pas_is_available() and the subsequent store do not seem
to be protected by a lock or atomic operation, which might allow the second
driver to silently overwrite the first.

Furthermore, if the else branch executes, and a concurrent thread
unregisters the ops immediately after the check, could ops_ptr->drv_name
trigger a NULL pointer dereference?

> diff --git a/drivers/firmware/qcom/qcom_pas.h b/drivers/firmware/qcom/qcom_pas.h
> new file mode 100644
> index 0000000000000..8643e2760602e
> --- /dev/null
> +++ b/drivers/firmware/qcom/qcom_pas.h
[ ... ]
> +struct qcom_pas_ops {
> +	const char *drv_name;
> +	struct device *dev;
> +	bool (*supported)(struct device *dev, u32 pas_id);

Is there a risk of a use-after-free here if the backend module is unloaded
while its callbacks are actively executing?

It doesn't appear that the framework pins the backend module using an owner
field and try_module_get(), or waits for ongoing callbacks to finish before
unregistration completes.

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