Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Removing the `qcom_scm_is_available()` check in `qcom_pas_probe()` exposes the driver to a NULL pointer dereference in `qcom_scm_assign_mem()`. Pre-existing issues: - [High] `pas->dtb_firmware` is leaked on the success path because `release_firmware()` is never called. - [High] If `qcom_pas_start()` fails after authenticating the DTB PAS, it never invokes `qcom_pas_shutdown()` for the DTB PAS, leaving it permanently running/authenticated in TrustZone. -- commit 410365720c51e458aee76cb1ae2177033dc208d2 Author: Sumit Garg <[email protected]> remoteproc: qcom_q6v5_pas: Switch over to generic PAS TZ APIs This commit switches the qcom_q6v5_pas client driver over to use generic PAS TZ APIs. This allows support for multiple TZ implementation backends, and updates the MDT loader PAS dependencies to prevent 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 [ ... ] > @@ -225,9 +226,9 @@ static int qcom_pas_load(struct rproc *rproc, const > struct firmware *fw) > pas->firmware = fw; > > if (pas->lite_pas_id) > - qcom_scm_pas_shutdown(pas->lite_pas_id); > + qcom_pas_shutdown(pas->lite_pas_id); > if (pas->lite_dtb_pas_id) > - qcom_scm_pas_shutdown(pas->lite_dtb_pas_id); > + qcom_pas_shutdown(pas->lite_dtb_pas_id); > > if (pas->dtb_pas_id) { > ret = request_firmware(&pas->dtb_firmware, > pas->dtb_firmware_name, pas->dev); [ ... ] > ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware, > pas->dtb_firmware_name, > pas->dtb_mem_region, > &pas->dtb_mem_reloc); > if (ret) > goto release_dtb_metadata; > } > > return 0; [Severity: High] This is a pre-existing issue, but does this code leak the DTB firmware buffer on the success path? If request_firmware() successfully allocates pas->dtb_firmware, it doesn't appear to be freed with release_firmware() here before returning 0, nor is it released anywhere in the teardown callbacks like qcom_pas_stop() or qcom_pas_unprepare(). Could this lead to memory exhaustion if the remoteproc is repeatedly started and stopped via sysfs? > > 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; [ ... ] > @@ -307,7 +308,7 @@ static int qcom_pas_start(struct rproc *rproc) > if (ret) > goto disable_px_supply; > > - ret = qcom_scm_pas_prepare_and_auth_reset(pas->dtb_pas_ctx); > + ret = qcom_pas_prepare_and_auth_reset(pas->dtb_pas_ctx); > if (ret) { > dev_err(pas->dev, > "failed to authenticate dtb image and release > reset\n"); > @@ -326,7 +327,7 @@ static int qcom_pas_start(struct rproc *rproc) > if (ret) > goto release_pas_metadata; [ ... ] > 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); [Severity: High] This isn't a bug introduced by this patch, but if a failure occurs after the DTB PAS is successfully authenticated, could this strand the TrustZone state? If qcom_pas_prepare_and_auth_reset(pas->dtb_pas_ctx) succeeds, but a subsequent step like qcom_mdt_pas_load() fails, the code jumps to the release_pas_metadata label. The error unwind doesn't seem to invoke qcom_pas_shutdown(pas->dtb_pas_id) for the DTB PAS, which might leave the DTB PAS session permanently active in TrustZone until the next reboot. > > unmap_dtb_carveout: > if (pas->dtb_pas_id) [ ... ] > @@ -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; > > fw_name = desc->firmware_name; [Severity: High] By changing this dependency check from qcom_scm_is_available() to qcom_pas_is_available(), is it possible to encounter a NULL pointer dereference later in the probe sequence? If the OP-TEE PAS backend is registered, qcom_pas_is_available() returns true, allowing probe to proceed even if the qcom_scm driver is deferred or unavailable. Later in the sequence, the code calls: qcom_pas_probe() qcom_pas_assign_memory_region() qcom_scm_assign_mem() If qcom_scm_assign_mem() is called unconditionally when region_assign_count > 0, it will attempt to use the __scm pointer to allocate TrustZone memory, which might be NULL if the SCM driver isn't available. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
