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 <[email protected]> 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/[email protected]?part=1
