Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Replacing the SCM availability check with a generic PAS check during probe breaks the strict SCM dependency for memory protection, leading to a NULL pointer dereference in `qcom_scm_assign_mem()`. - [High] Missing module reference counting in the generic PAS API exposes client drivers to a TOCTOU race and use-after-free if the backend module is unloaded. --
commit f9f49708d56de20022405acdb58867dd0cffe7bb Author: Sumit Garg <[email protected]> remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs Switch qcom_q6v5_mss 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. > 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) { > - 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] Is there a time-of-check to time-of-use race in the generic PAS API that could lead to a use-after-free or NULL pointer dereference? The client driver switches to using the newly introduced generic PAS API without any mechanism to prevent backend unloads. Looking at qcom_pas_mem_setup() in drivers/firmware/qcom/qcom_pas.c, the API locklessly dereferences ops_ptr after checking it, without holding a module reference or synchronization: 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); } If a privileged user unloads the backend module (like qcom_pas_tee) concurrently with the client driver starting the remoteproc, ops_ptr could be set to NULL between the check and the dereference. Does the generic PAS API need module reference counting (like try_module_get()) or device links to prevent the backend module from being unloaded while in use? > 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] Does this change break the memory protection dependency on the SCM driver? If desc->need_mem_protection is true, but the SCM driver is not available (for example, if the OP-TEE PAS backend probes first, or if SCM fails to probe), q6v5_probe() will now observe qcom_pas_is_available() as true and incorrectly succeed without deferring probe for SCM. When the remoteproc is later started, q6v5_xfer_mem_ownership() unconditionally calls qcom_scm_assign_mem(), which directly dereferences the uninitialized __scm pointer: drivers/firmware/qcom/qcom_scm.c:qcom_scm_assign_mem() { ... void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, ptr_sz, GFP_KERNEL); ... } Could this lead to a NULL pointer dereference in qcom_scm_assign_mem() on configurations that use OP-TEE PAS or have SCM probe delayed? > return -EPROBE_DEFER; > > mba_image = desc->hexagon_mba_image; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
