Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] qcom_pas_tee_remove() unconditionally unregisters the global PAS ops, which can corrupt system state by deregistering another active backend. - [High] qcom_pas_tee_get_rsc_table() fails with -EINVAL and aborts initialization when handling a zero-sized resource table. - [High] Broken TEE size-query logic in qcom_pas_tee_get_rsc_table() aborts on TEEC_ERROR_SHORT_BUFFER and passes a NULL buffer with a non-zero size. - [High] qcom_pas_tee_get_rsc_table() silently discards the resource table if the TA requests a size smaller than input_rt_size. - [High] Lack of synchronization and module refcounting on the global ops_ptr leads to TOCTOU and Use-After-Free bugs. - [High] Unvalidated TA-provided size leads to an out-of-bounds read of kernel memory in qcom_pas_tee_get_rsc_table(). --
Note: The format of this report is altered due to system recitation restrictions. Instead of inline diff quotes, a free-form summary of the findings is provided below. commit c175d6ee38b74c2a77d95052360287597c771c21 Author: Sumit Garg <[email protected]> Subject: firmware: qcom: Add a PAS TEE service This commit adds support for the Peripheral Authentication Service (PAS) driver based on the TEE bus with OP-TEE. It provides the backend PAS service implementation. [Severity: High] In drivers/firmware/qcom/qcom_pas_tee.c during driver removal, does qcom_pas_tee_remove() forcefully unregister the global PAS ops even if this driver wasn't the one that successfully registered them? Looking at qcom_pas_tee_probe(), it calls qcom_pas_ops_register() which returns void and only logs an error if another provider (like qcom_scm) is already registered. This allows the probe to succeed without aborting. However, upon module removal, qcom_pas_tee_remove() unconditionally calls qcom_pas_ops_unregister() which wipes the global pointer, potentially breaking subsequent operations for the actual active backend. [Severity: High] Does qcom_pas_tee_get_rsc_table() fail the initialization sequence for peripherals without dynamic resource requirements? If input_rt_size is 0 and the TA also returns a required size of 0, the driver allocates a 0-sized shared memory object and calls tee_shm_get_va(rt_shm, 0). This triggers a size check failure internally in the TEE subsystem because the offset (0) is not less than the size (0), returning -EINVAL and aborting the setup sequence. [Severity: High] Could the TEE size-query logic in qcom_pas_tee_get_rsc_table() pass a NULL buffer with a non-zero size to the TA? During the initial invocation, the driver sets the parameter size to input_rt_size but does not allocate a shared memory buffer, passing NULL. Additionally, if the TA correctly responds with TEEC_ERROR_SHORT_BUFFER to request a larger buffer, the driver treats any non-zero return code as a fatal error and aborts instead of proceeding to allocate the buffer. [Severity: High] Will qcom_pas_tee_get_rsc_table() silently discard the resource table if the TA requires a smaller size than the original input_rt_size? If the TA optimizes or removes entries and returns a required size strictly less than input_rt_size, the driver falls into the else block, sets the output size to 0, and returns NULL. This causes the original static resource table to be lost. [Severity: High] Are there missing concurrency protections for the global ops_ptr in the new core driver design? In wrappers like qcom_pas_shutdown(), the global ops_ptr is read twice (first to check for NULL, then to dereference) without any locking or RCU. If qcom_pas_tee_remove() is called concurrently, it sets ops_ptr to NULL, which could cause a NULL pointer dereference. Furthermore, since the operations structure lacks an owner field to pin the module in memory, the backend module could be unloaded while its functions are actively executing on another CPU. [Severity: High] Can qcom_pas_tee_get_rsc_table() read out of bounds from kernel memory if the TA provides an unexpectedly large size? During the second TA invocation, the driver unconditionally trusts the size returned by the TA and uses it in kmemdup(). If the returned size exceeds the originally allocated size of the rt_shm buffer, kmemdup() will read past the bounds of the buffer on the kernel heap, leaking sensitive memory. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
