Re: [PATCH v7 02/15] firmware: qcom: Add a generic PAS service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] API wrapper functions access the global `ops_ptr` without `READ_ONCE()` or memory barriers, leading to potential double-fetch NULL pointer dereferences.
- [High] Racy check-then-set registration of `ops_ptr` allows concurrent registrations to overwrite each other and introduces a TOCTOU panic in the error path.
- [High] Missing module reference counting for the backend PAS provider allows it to be unloaded while actively executing callbacks, leading to Use-After-Free crashes.
- [Medium] API wrappers unconditionally invoke backend callbacks without verifying they are implemented.
- [Medium] Unbinding the PAS backend while client drivers hold active contexts causes silent metadata resource leaks.
- [Medium] `qcom_pas_ops_register` returns `void`, failing to communicate registration failures to the caller.
--

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

firmware: qcom: Add a generic PAS service

This commit adds a generic Peripheral Authentication Service (PAS) interface.
It acts as a common layer for TZ backends, supporting both legacy SCM calls
and newer TEE bus based PAS implementations.

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

[Severity: High]
Since ops_ptr is a global variable modified locklessly, can the compiler emit
multiple loads here?

If ops_ptr is unregistered concurrently between the !ops_ptr check and the
dereference in the return statement, the second load could yield NULL, causing
a kernel panic. Should this load ops_ptr exactly once into a local variable
using smp_load_acquire() before checking and using it?

[Severity: Medium]
Also, does this function (and the other API wrappers) need to verify that
ops_ptr->init_image is actually implemented before calling it?

If a backend fails to provide an implementation, this would cause a NULL
pointer dereference.

[ ... ]
> +void qcom_pas_metadata_release(struct qcom_pas_context *ctx)
> +{
> +	if (!ops_ptr || !ctx || !ctx->ptr)
> +		return;
> +
> +	ops_ptr->metadata_release(ops_ptr->dev, ctx);
> +}

[Severity: Medium]
If the PAS backend unregisters and sets ops_ptr to NULL while client drivers
still hold active contexts, will this early return permanently leak the
metadata resources (like DMA mappings) since the backend's metadata_release
callback is never executed?

[ ... ]
> +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);
> +}

[Severity: Medium]
Is there a way for the caller to detect if registration failed since this
function returns void? A backend driver might incorrectly assume it has
successfully initialized if another driver is already registered.

[Severity: High]
Without a lock, could multiple drivers register concurrently, all observe
a NULL ops_ptr, and overwrite each other?

Additionally, in the else branch, if ops_ptr is cleared by a concurrent
unregistration immediately before the read of ops_ptr->drv_name, could
this cause 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;

[Severity: High]
Does this structure need a struct module *owner field?

Without module reference counting (using try_module_get() in the wrapper
functions), could the backend module be unloaded while a client thread is
actively executing inside one of these callbacks, leading to a use-after-free
crash?

> +	bool (*supported)(struct device *dev, u32 pas_id);
> +	int (*init_image)(struct device *dev, u32 pas_id, const void *metadata,
> +			  size_t size, struct qcom_pas_context *ctx);
[ ... ]

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