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

[Sumit Garg <sumit.garg@kernel.org>]

On Tue, May 19, 2026 at 07:33:48AM +0000, sashiko-bot@kernel.org wrote:
> 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?

PAS backend isn't expected to unregister during the boot power cycle.

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

Only one PAS backend is expected to be supported on a system.

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

The PAS backend isn't expected to be unloaded at runtime.

-Sumit