Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Replacing `qcom_scm_is_available()` with `qcom_pas_is_available()` removes the SCM dependency check, leading to a NULL pointer dereference during memory protection. - [High] The generic PAS API removes the hard module dependency on the backend provider, exposing client drivers to a TOCTOU race condition and Use-After-Free. Pre-existing issues: - [High] Missing initialization of `need_pas_mem_setup` makes PAS memory setup silently skipped. -- commit 286e03507d8f96a33a10d31ad2cb432432734d78 Author: Sumit Garg <[email protected]> Subject: remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs This commit switches the qcom_q6v5_mss client driver to use the generic PAS TZ APIs. This enables support for multiple TZ implementation backends, such as QTEE based SCM PAS service and OP-TEE based PAS service. > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c > b/drivers/remoteproc/qcom_q6v5_mss.c > index ae78f5c7c1b69..96888007faa88 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c [ ... ] > @@ -1480,7 +1481,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > } > > if (qproc->need_pas_mem_setup) { [Severity: High] This is a pre-existing issue, but I noticed that need_pas_mem_setup is never populated from the match descriptor in q6v5_probe(). Since qproc is allocated via devm_rproc_alloc() and zero-initialized, this boolean will always be false. Does this prevent qcom_pas_mem_setup() from ever being called even on platforms that require it? > - ret = qcom_scm_pas_mem_setup(MPSS_PAS_ID, qproc->mpss_phys, > qproc->mpss_size); > + ret = qcom_pas_mem_setup(MPSS_PAS_ID, qproc->mpss_phys, > qproc->mpss_size); [Severity: High] Does this new generic PAS API safely handle backend module unloading? Previously, calling qcom_scm_pas_mem_setup() created a hard symbol dependency, preventing the backend module from being unloaded. The generic framework uses a lockless ops_ptr without acquiring module references: drivers/firmware/qcom/qcom_pas.c:qcom_pas_mem_setup() { if (!ops_ptr) return -ENODEV; return ops_ptr->mem_setup(ops_ptr->dev, pas_id, addr, size); } Could the backend module be unloaded after the ops_ptr check but before or during the mem_setup() call, leading to a use-after-free or NULL pointer dereference? > if (ret) { > dev_err(qproc->dev, > "setting up mpss memory failed: %d\n", ret); [ ... ] > @@ -2077,7 +2078,7 @@ static int q6v5_probe(struct platform_device *pdev) > if (!desc) > return -EINVAL; > > - if (desc->need_mem_protection && !qcom_scm_is_available()) > + if (desc->need_mem_protection && !qcom_pas_is_available()) [Severity: High] Is it safe to replace the SCM availability check with the PAS availability check here? If need_mem_protection is true, q6v5_mpss_load() will unconditionally call q6v5_xfer_mem_ownership(), which eventually relies on qcom_scm_assign_mem() and dereferences the internal SCM structures. If a non-SCM PAS backend is available but SCM is uninitialized, the PAS check will succeed here, but the subsequent memory protection operations will trigger a NULL pointer dereference. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
