Re: [PATCH v8 01/14] 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] API wrapper functions perform unprotected back-to-back reads of the global `ops_ptr`, leading to a Time-of-Check to Time-of-Use (TOCTOU) NULL pointer dereference.
- [High] The PAS framework lacks module reference counting or lifecycle protection for the backend provider, making it vulnerable to Use-After-Free if the provider is unloaded.
- [High] `qcom_pas_ops_register()` contains a lockless race condition that allows concurrent registrations to silently overwrite each other, and an unsafe error path that risks a NULL pointer dereference.
--

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

firmware: qcom: Add a generic PAS service

This commit enables the generic Peripheral Authentication Service (PAS)
provided by the firmware. It acts as a common layer with different TZ
backends plugged in, replacing splintered legacy SCM calls.

> diff --git a/drivers/firmware/qcom/qcom_pas.c b/drivers/firmware/qcom/qcom_pas.c
> --- /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]
Could this result in a Time-of-Check to Time-of-Use (TOCTOU) NULL pointer
dereference?

Since ops_ptr is accessed locklessly without READ_ONCE(), the compiler
could emit two separate memory reads. If a concurrent thread executes
qcom_pas_ops_unregister() and sets ops_ptr to NULL between the check and
the dereference, it looks like a kernel panic would occur.

Would it be safer to fetch ops_ptr into a local variable using
READ_ONCE() before checking and using it?

[ ... ]

> +struct resource_table *qcom_pas_get_rsc_table(struct qcom_pas_context *ctx,
> +					      void *input_rt,
> +					      size_t input_rt_size,
> +					      size_t *output_rt_size)
> +{
> +	if (!ops_ptr)
> +		return ERR_PTR(-ENODEV);
> +	if (!ctx)
> +		return ERR_PTR(-EINVAL);
> +
> +	return ops_ptr->get_rsc_table(ops_ptr->dev, ctx, input_rt,
> +				      input_rt_size, output_rt_size);
> +}

[Severity: High]
Does this function suffer from the same Time-of-Check to Time-of-Use
NULL pointer dereference on ops_ptr as qcom_pas_init_image()?

[ ... ]

> +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: High]
Is this lockless check-then-set logic vulnerable to a race condition?

Two backend drivers probing concurrently might both see
qcom_pas_is_available() as false and write to ops_ptr, allowing the second
driver to silently overwrite the first.

Additionally, in the else branch, what happens if ops_ptr is concurrently
set to NULL by qcom_pas_ops_unregister()? Does the unlocked read of
ops_ptr->drv_name risk a NULL pointer dereference?

> diff --git a/drivers/firmware/qcom/qcom_pas.h b/drivers/firmware/qcom/qcom_pas.h
> --- /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);

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

The PAS framework doesn't appear to perform module reference counting. If
the backend module providing these operations is unloaded while a remoteproc
client is actively invoking a PAS API, the backend's code and data would be
unmapped while the CPU is still executing or returning from it, leading to
a Use-After-Free.

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