Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Calling `qcom_scm_mem_protect_video_var()` without ensuring SCM is available causes a NULL pointer dereference. - [High] Transition to generic PAS API introduces a Use-After-Free race condition due to missing module reference management. --
commit c3e79e117e7109763310cc691fd2c7bcd036f2c0 Author: Sumit Garg <[email protected]> media: qcom: Switch to generic PAS TZ APIs Switch qcom media client drivers 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/media/platform/qcom/venus/firmware.c > b/drivers/media/platform/qcom/venus/firmware.c > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c [ ... ] > @@ -218,7 +219,7 @@ int venus_boot(struct venus_core *core) > int ret; > > if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) || > - (core->use_tz && !qcom_scm_is_available())) > + (core->use_tz && !qcom_pas_is_available())) > return -EPROBE_DEFER; [Severity: High] By replacing qcom_scm_is_available() with qcom_pas_is_available(), we no longer guarantee that the SCM driver has probed. However, later in this function (and similarly in iris_fw_load), we still unconditionally call the SCM-specific API: drivers/media/platform/qcom/venus/firmware.c:venus_boot() { ... ret = qcom_scm_mem_protect_video_var(res->cp_start, res->cp_size, res->cp_nonpixel_start, res->cp_nonpixel_size); ... } If a non-SCM backend like OP-TEE is used, will this cause a NULL pointer dereference in qcom_scm_call() because the global __scm pointer was never initialized? drivers/firmware/qcom/qcom_scm.c:qcom_scm_mem_protect_video_var() { ... ret = qcom_scm_call(__scm->dev, &desc, &res); ... } [ ... ] > @@ -236,7 +237,7 @@ int venus_boot(struct venus_core *core) > core->fw.mem_phys = mem_phys; > > if (core->use_tz) > - ret = qcom_scm_pas_auth_and_reset(VENUS_PAS_ID); > + ret = qcom_pas_auth_and_reset(VENUS_PAS_ID); [Severity: High] Does switching to the generic PAS API introduce a use-after-free race condition if the backend module is unloaded concurrently? Looking at the generic PAS implementation: drivers/firmware/qcom/qcom_pas.c:qcom_pas_auth_and_reset() { if (!ops_ptr) return -ENODEV; return ops_ptr->auth_and_reset(ops_ptr->dev, pas_id); } If the generic PAS backend is built as a module and unloaded by userspace right after the ops_ptr check passes, could ops_ptr->auth_and_reset jump to unmapped memory? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
