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