Re: [PATCH v6 11/16] 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:
- [High] Unconditional use of SCM-specific API (qcom_scm_mem_protect_video_var) will cause a NULL pointer dereference when using a non-SCM TrustZone backend.
- [High] Migrating to the generic PAS API exposes a TOCTOU race and Use-After-Free due to a lack of backend provider lifetime management.
--

commit 2d2fe5038803b9390438e02f3c0f53b945cc9bdc
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

media: qcom: Switch to generic PAS TZ APIs

This commit switches the Qualcomm media client drivers to use the generic
PAS TZ APIs instead of the SCM-specific ones. This allows the drivers to
support multiple TrustZone backend implementations such as QTEE and OP-TEE.

> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index 5f408024e967f..b3c5281aea919 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
[ ... ]
> @@ -79,7 +80,7 @@ int iris_fw_load(struct iris_core *core)
>  		return -ENOMEM;
>  	}
>  
> -	ret = qcom_scm_pas_auth_and_reset(core->iris_platform_data->pas_id);
> +	ret = qcom_pas_auth_and_reset(core->iris_platform_data->pas_id);

Does moving to the generic PAS API introduce a race condition with module
unloading?

Looking at the generic PAS wrapper qcom_pas_auth_and_reset() in
drivers/firmware/qcom/qcom_pas.c:

int qcom_pas_auth_and_reset(u32 pas_id)
{
	if (!ops_ptr)
		return -ENODEV;

	return ops_ptr->auth_and_reset(ops_ptr->dev, pas_id);
}

If the PAS backend module is unloaded concurrently, ops_ptr could be set to
null after the check but before the function pointer is invoked. Do the
client drivers need to hold a reference count to pin the backend provider
in memory before calling these APIs to avoid a use-after-free?

[ ... ]
> 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;

If the platform uses a non-SCM TrustZone backend like OP-TEE,
qcom_pas_is_available() will return true, but the qcom_scm subsystem
might not be probed.

Will this lead to a null pointer dereference later in venus_boot() when
it calls the SCM-specific API?

venus_boot() {
    ...
	if (core->use_tz && res->cp_size) {
		ret = qcom_scm_mem_protect_video_var(res->cp_start,
						     res->cp_size,
						     res->cp_nonpixel_start,
						     res->cp_nonpixel_size);
	}
    ...
}

Looking at qcom_scm_mem_protect_video_var(), it executes
qcom_scm_call(__scm->dev, ...). If __scm is null because the SCM
subsystem is absent, this dereference might cause a panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518072856.22790-1-sumit.garg@kernel.org?part=11