Re: [PATCH v9 09/14] media: qcom: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

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 <sumit.garg@oss.qualcomm.com>

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/20260702115835.167602-1-sumit.garg@kernel.org?part=9