Re: [PATCH v9 08/14] drm/msm: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Generic PAS TEE backend maps TA-level errors to -EINVAL, breaking the DRM MSM fallback logic.
- [High] TOCTOU race condition in generic PAS API wrappers leading to NULL pointer dereference.
- [High] Unconditional PAS availability check causes infinite probe deferral on platforms without PAS support.

Pre-existing issues:
- [High] The MSM DRM driver propagates -EOPNOTSUPP instead of -ENODEV, fatally failing GPU initialization.
--

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

drm/msm: Switch to generic PAS TZ APIs
    
Switch drm/msm 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/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index c62c45bb0ddbb..489462065ea95 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c

[ ... ]

> @@ -146,10 +147,10 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
>  		goto out;
>  
>  	/* Send the image to the secure world */
> -	ret = qcom_scm_pas_auth_and_reset(pasid);
> +	ret = qcom_pas_auth_and_reset(pasid);

[Severity: High]
Does this new PAS API wrapper have a TOCTOU race condition?

If the PAS backend unregisters concurrently, could ops_ptr become NULL
between the check and the dereference in qcom_pas_auth_and_reset()?

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 scm call returns -EOPNOTSUPP we assume that this target
> +	 * If the pas call returns -EOPNOTSUPP we assume that this target
>  	 * doesn't need/support the zap shader so quietly fail
>  	 */
>  	if (ret == -EOPNOTSUPP)

[Severity: High]
Is the DRM MSM fallback logic preserved with this generic backend?

The driver expects -EOPNOTSUPP to gracefully handle unsupported zap shaders.

If the generic PAS TEE backend's __qcom_pas_tee_auth_and_reset()
unconditionally maps all TA errors to -EINVAL, will this cause GPU probe
to fail fatally on TEE platforms where the zap shader is not supported?

drivers/firmware/qcom/qcom_pas_tee.c:__qcom_pas_tee_auth_and_reset() {
	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
	if (ret < 0 || inv_arg.ret != 0) {
		...
		return ret ?: -EINVAL;
	}
}

[Severity: High]
This isn't a bug introduced by this patch, but does this function propagate
-EOPNOTSUPP instead of -ENODEV?

If qcom_pas_auth_and_reset() returns -EOPNOTSUPP, the code sets
zap_available = false but leaves ret = -EOPNOTSUPP.

Since callers explicitly check for -ENODEV to fall back to
SECVID_TRUST_CNTL, could this unmapped -EOPNOTSUPP abort hardware
initialization?

>  		zap_available = false;
>  	else if (ret)
>  		DRM_DEV_ERROR(dev, "Unable to authorize the image\n");

[ ... ]

> @@ -175,9 +176,9 @@ int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid)
>  	if (!zap_available)
>  		return -ENODEV;
>  
> -	/* We need SCM to be able to load the firmware */
> -	if (!qcom_scm_is_available()) {
> -		DRM_DEV_ERROR(&pdev->dev, "SCM is not available\n");
> +	/* We need PAS to be able to load the firmware */
> +	if (!qcom_pas_is_available()) {
> +		DRM_DEV_ERROR(&pdev->dev, "PAS is not available\n");
>  		return -EPROBE_DEFER;
>  	}

[Severity: High]
Could this unconditional PAS availability check cause infinite probe
deferral on platforms without PAS support?

By checking qcom_pas_is_available() before checking if the zap-shader DT
node exists (which happens inside zap_shader_load_mdt()), do platforms
that lack PAS capabilities fail to reach the DT node check and defer
indefinitely?

Previously, qcom_scm_is_available() returned true on these platforms,
allowing the driver to gracefully return -ENODEV when the DT node was
missing.

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