Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] Unconditional call to SCM-specific qcom_scm_assign_mem() causes NULL pointer dereference with non-SCM backends. - [High] Missing module reference counting in the generic PAS API introduces a Use-After-Free and race condition. Pre-existing issues: - [High] Asynchronous pm_runtime_put() is canceled during device removal, leaving hardware powered on. - [High] The DTB firmware buffer is leaked on the successful boot path. - [High] The DTB PAS backend authentication session is not shut down if starting the main image fails. -- commit 5fe417b2557886cf59a2b2dcdc429ef1c4165522 Author: Sumit Garg <[email protected]> remoteproc: qcom_q6v5_pas: Switch over to generic PAS TZ APIs Switch qcom_q6v5_pas client driver over to generic PAS TZ APIs. Generic PAS TZ service allows to support multiple TZ implementation backends like QTEE based SCM PAS service, OP-TEE based PAS service and any further future TZ backend service. Since qcom_q6v5_pas depends on MDT loader for PAS firmware loading, it has to be switched over to generic PAS APIs in this commit to avoid any build issues. > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c > b/drivers/remoteproc/qcom_q6v5_pas.c > index 808e9609988d3..9eb1c4f6c2ab4 100644 > --- a/drivers/remoteproc/qcom_q6v5_pas.c > +++ b/drivers/remoteproc/qcom_q6v5_pas.c [ ... ] > @@ -247,7 +248,7 @@ static int qcom_pas_load(struct rproc *rproc, const > struct firmware *fw) > return 0; > [Severity: High] This is a pre-existing issue, but does this leak the dtb_firmware buffer on the successful boot path? If the loading process succeeds, the function returns 0 without releasing the buffer allocated earlier by request_firmware(). Since the remoteproc core only automatically frees the main firmware buffer, wouldn't the auxiliary dtb_firmware buffer be left allocated every time the remote processor boots? > release_dtb_metadata: > - qcom_scm_pas_metadata_release(pas->dtb_pas_ctx); > + qcom_pas_metadata_release(pas->dtb_pas_ctx); > release_firmware(pas->dtb_firmware); > > return ret; [ ... ] > @@ -352,9 +353,9 @@ static int qcom_pas_start(struct rproc *rproc) > unmap_carveout: > qcom_pas_unmap_carveout(rproc, pas->mem_phys, pas->mem_size); > release_pas_metadata: > - qcom_scm_pas_metadata_release(pas->pas_ctx); > + qcom_pas_metadata_release(pas->pas_ctx); > if (pas->dtb_pas_id) > - qcom_scm_pas_metadata_release(pas->dtb_pas_ctx); > + qcom_pas_metadata_release(pas->dtb_pas_ctx); > > unmap_dtb_carveout: > if (pas->dtb_pas_id) [Severity: High] This is a pre-existing issue, but does the error rollback path miss shutting down the DTB PAS backend? If a subsequent step in qcom_pas_start() fails after the DTB image is mapped and authenticated, the error path jumps to release_pas_metadata and unmap_dtb_carveout. However, it doesn't appear to invoke qcom_pas_shutdown() for the dtb_pas_id. Could this leave the DTB backend authentication session active and prevent the subsystem from loading again until the device reboots? [ ... ] > @@ -403,7 +404,7 @@ static int qcom_pas_stop(struct rproc *rproc) > if (ret == -ETIMEDOUT) > dev_err(pas->dev, "timed out on wait\n"); > > - ret = qcom_scm_pas_shutdown(pas->pas_id); > + ret = qcom_pas_shutdown(pas->pas_id); [Severity: High] Could this lead to memory corruption if the backend module is unloaded? With the switch to the generic PAS API, struct qcom_pas_ops lacks an owner field, and the wrapper functions don't take a reference to the backend module using try_module_get(). If the backend is unloaded concurrently, the global ops_ptr would be cleared, causing qcom_pas_shutdown() to return -ENODEV instead of shutting down the hardware. Wouldn't the remote processor continue executing while its carveout memory is unmapped and freed by Linux? > if (ret && pas->decrypt_shutdown) > ret = qcom_pas_shutdown_poll_decrypt(pas); > > if (ret) [ ... ] > @@ -743,7 +744,7 @@ static int qcom_pas_probe(struct platform_device *pdev) > if (!desc) > return -EINVAL; > > - if (!qcom_scm_is_available()) > + if (!qcom_pas_is_available()) > return -EPROBE_DEFER; > [Severity: High] Can this cause a NULL pointer dereference with non-SCM backends? By broadening the check to qcom_pas_is_available(), probe can proceed when backed by non-SCM services like OP-TEE. However, further down in the probe sequence, qcom_pas_assign_memory_region() unconditionally calls the SCM-specific qcom_scm_assign_mem() (if region_assign_idx is present). If qcom_scm did not probe, the internal pointer inside qcom_scm_assign_mem() would be NULL, leading to a kernel panic when dereferenced. > fw_name = desc->firmware_name; > ret = of_property_read_string(pdev->dev.of_node, "firmware-name", [ ... ] > @@ -835,16 +836,16 @@ static int qcom_pas_probe(struct platform_device *pdev) > > qcom_add_ssr_subdev(rproc, &pas->ssr_subdev, desc->ssr_name); > > - pas->pas_ctx = devm_qcom_scm_pas_context_alloc(pas->dev, pas->pas_id, > - pas->mem_phys, > pas->mem_size); > + pas->pas_ctx = devm_qcom_pas_context_alloc(pas->dev, pas->pas_id, > + pas->mem_phys, > pas->mem_size); > if (IS_ERR(pas->pas_ctx)) { > ret = PTR_ERR(pas->pas_ctx); > goto remove_ssr_sysmon; > } > [Severity: High] This isn't a bug introduced by this patch, but does the device teardown path cancel pending asynchronous suspends? If probe fails and jumps to the teardown labels, qcom_pas_pds_disable() is called, which drops the usage counter for power domains using the asynchronous pm_runtime_put(). This is followed shortly by pm_runtime_disable() in qcom_pas_pds_detach(). Since pm_runtime_disable() cancels pending asynchronous suspend work, wouldn't this leave the hardware in an incorrect active power state? Should the synchronous pm_runtime_put_sync() be used instead? > - pas->dtb_pas_ctx = devm_qcom_scm_pas_context_alloc(pas->dev, > pas->dtb_pas_id, > - pas->dtb_mem_phys, > - pas->dtb_mem_size); > + pas->dtb_pas_ctx = devm_qcom_pas_context_alloc(pas->dev, > pas->dtb_pas_id, > + pas->dtb_mem_phys, > + pas->dtb_mem_size); > if (IS_ERR(pas->dtb_pas_ctx)) { > ret = PTR_ERR(pas->dtb_pas_ctx); > goto remove_ssr_sysmon; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
