Re: [PATCH v9 01/14] firmware: qcom: Add a generic PAS service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] TOCTOU data race and missing memory barriers in PAS wrapper functions.
- [High] Check-then-set race condition in qcom_pas_ops_register() allows concurrent overwrites and unsafe reads.
- [High] Missing module lifecycle management and reference counting for the PAS backend provider.
- [Medium] Resource leak when calling qcom_pas_metadata_release after provider unregistration.
- [Medium] Missing NULL pointer validation for mandatory ops callbacks before execution.
--

Note: This review is provided in a free-form summary format without direct
patch quotes due to system recitation restrictions.

[Severity: High]
In wrapper functions like qcom_pas_init_image() and qcom_pas_get_rsc_table(),
the code checks if ops_ptr is NULL using a plain load, and then subsequently
dereferences ops_ptr to call the provider.

Since these loads are not protected by a mechanism to fetch into a local
variable, could the compiler fetch the pointer multiple times from memory?
If the provider module is unloaded concurrently, might ops_ptr become NULL
between the check and the dereference, leading to a crash?

[Severity: High]
Looking at qcom_pas_ops_register(), there appears to be a check-then-set race
condition. If two threads call this simultaneously, both might check the
availability, see NULL, and both perform the store release, allowing
concurrent registrations to overwrite each other.

Additionally, in the error path for this function, it directly reads the
driver name from ops_ptr. If qcom_pas_ops_unregister() is called concurrently,
could this pointer become NULL and cause a crash when printing the error?

[Severity: High]
The qcom_pas_ops structure lacks a module owner field for lifecycle
management.

When wrapper functions like qcom_pas_auth_and_reset() invoke these callbacks,
they do not appear to pin the provider module in memory using a reference
count. If the backend provider module is unloaded while its callbacks are
actively executing, will the kernel jump to unmapped memory?

[Severity: Medium]
In qcom_pas_metadata_release(), there is an early return if the operations
pointer is NULL.

If the backend provider is unregistered before the client releases its context,
this bypasses the metadata release callback entirely. Does this early return
permanently leak resources, such as DMA mappings or shared memory, that were
allocated by the client during initialization?

[Severity: Medium]
Wrapper functions like qcom_pas_shutdown() directly execute callbacks from the
operations pointer without verifying if the specific function pointer is
populated.

If a registered backend provider fails to populate all mandatory callback
function pointers during registration, could this lead to a NULL pointer
dereference when the wrapper attempts to execute them?

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