Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Calling qcom_scm_mem_protect_video_var() after changing the availability check to qcom_pas_is_available() causes a NULL pointer dereference on non-SCM platforms. Pre-existing issues: - [High] The iris driver misses the mandatory qcom_pas_is_available() check, failing to defer probe and missing an smp_load_acquire() memory barrier when accessing the generic PAS API. -- commit d77a18836b2d6ae833e5833c165d79299ffd8a66 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/iris/iris_firmware.c > b/drivers/media/platform/qcom/iris/iris_firmware.c > index 1a476146d7580..ea9654dd679e1 100644 > --- a/drivers/media/platform/qcom/iris/iris_firmware.c > +++ b/drivers/media/platform/qcom/iris/iris_firmware.c [ ... ] > @@ -80,7 +81,7 @@ int iris_fw_load(struct iris_core *core) > return -ENOMEM; > } > > - ret = qcom_scm_pas_auth_and_reset(IRIS_PAS_ID); > + ret = qcom_pas_auth_and_reset(IRIS_PAS_ID); [Severity: High] This is a pre-existing issue, but it seems the iris driver is missing a check for qcom_pas_is_available() during probe to defer if the backend isn't ready. Because it misses the qcom_pas_is_available() check, it misses the mandatory smp_load_acquire() memory barrier. When userspace opens the device, iris_fw_load() invokes qcom_pas_auth_and_reset(). This function performs a plain lockless load of ops_ptr. Is it possible for a weakly ordered architecture to observe a non-NULL ops_ptr but uninitialized function pointers within the ops structure here, causing a kernel panic? > if (ret) { > dev_err(core->dev, "auth and reset failed: %d\n", ret); > return ret; > @@ -94,7 +95,7 @@ int iris_fw_load(struct iris_core *core) > > cp_config->cp_nonpixel_size); > if (ret) { > dev_err(core->dev, "qcom_scm_mem_protect_video_var > failed: %d\n", ret); > - qcom_scm_pas_shutdown(IRIS_PAS_ID); > + qcom_pas_shutdown(IRIS_PAS_ID); [Severity: High] If the PAS backend is non-SCM (like OP-TEE), the SCM driver will not be probed. Since qcom_scm_mem_protect_video_var() is still called unconditionally just before this error handling block, will it dereference a NULL __scm pointer (__scm->dev) and crash the kernel? > return ret; > } > } > diff --git a/drivers/media/platform/qcom/venus/firmware.c > b/drivers/media/platform/qcom/venus/firmware.c > index 1de7436713ed9..3a38ff985822f 100644 > --- 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; > > ret = of_property_read_string_index(dev->of_node, "firmware-name", 0, [ ... ] > @@ -259,7 +260,7 @@ int venus_boot(struct venus_core *core) > res->cp_nonpixel_start, > res->cp_nonpixel_size); > if (ret) { > - qcom_scm_pas_shutdown(VENUS_PAS_ID); > + qcom_pas_shutdown(VENUS_PAS_ID); [Severity: High] Similar to the iris driver, the availability check was changed to qcom_pas_is_available(), but qcom_scm_mem_protect_video_var() is still called unconditionally when cp_size > 0. Will this also result in a NULL pointer dereference on non-SCM platforms? > dev_err(dev, "set virtual address ranges fail (%d)\n", > ret); > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
