[PATCH] drm/msm/adreno: Only check for PAS when a zap shader is present

[PATCH] drm/msm/adreno: Only check for PAS when a zap shader is present

[Paul Hollinsky]

Commit 0be72be03ca7 ("drm/msm: Switch to generic PAS TZ APIs") replaced
the qcom_scm_is_available() check in adreno_zap_shader_load() with
qcom_pas_is_available(). These are not equivalent: the former reports
whether the SCM transport is up, the latter whether the TrustZone
firmware implements the peripheral authentication service.

On SC7180 Chromebooks (trogdor) TZ does not implement PAS at all. SCM
call-availability queries return 0 for every PAS command while other
services answer normally:

  svc 0x06 cmd 0x01 IS_CALL_AVAIL    -> 1
  svc 0x02 cmd 0x01 PAS_INIT_IMAGE   -> 0
  svc 0x02 cmd 0x05 PAS_AUTH_RESET   -> 0
  svc 0x02 cmd 0x07 PAS_IS_SUPPORTED -> 0
  svc 0x0c cmd 0x16 MP_ASSIGN        -> 1
  svc 0x05 cmd 0x01 IO_READ          -> 1

so qcom_scm_probe() never registers a PAS backend and
qcom_pas_is_available() is false for the lifetime of the boot.

That on its own need not matter, because sc7180-trogdor.dtsi does
/delete-node/ &gpu_zap_shader;, and the intended path for such a board
is for zap_shader_load_mdt() to find no zap-shader child, clear
zap_available, return -ENODEV, and let the caller fall back to
SECVID_TRUST_CNTL.

The problem is the ordering. zap_available is a static initialised to
true and is only ever cleared inside zap_shader_load_mdt(), but
adreno_zap_shader_load() consults PAS before calling it. The discovery
that decides whether a zap shader is needed at all can therefore never
run, the flag is never cleared, and every call returns -EPROBE_DEFER:

  adreno 5000000.gpu: [drm:adreno_zap_shader_load] *ERROR* PAS is not available
  msm_dpu ae01000.display-controller: [drm:adreno_load_gpu] *ERROR* gpu hw init failed: -517

Nothing retries that deferral, either. adreno_zap_shader_load() is
called from a6xx_hw_init() rather than from probe, so the -EPROBE_DEFER
is not a probe return value: it propagates up until adreno_load_gpu()
returns NULL. load_gpu() re-attempts on every DRM open while priv->gpu
is NULL, each open fails identically, and PAS cannot become available in
between - which is why the error repeats and userspace stays on
llvmpipe.

Move the availability check into zap_shader_load_mdt(), behind the
zap-shader node lookup, so the driver only consults PAS once it knows it
needs PAS. Boards with no zap-shader node take the intended -ENODEV
fallback without ever asking, and boards that do have one keep the
qcom_pas_is_available() gate.

Fixes: 0be72be03ca7 ("drm/msm: Switch to generic PAS TZ APIs")
Link: https://lore.kernel.org/r/20260808034716.58888-1-phollinsky@holtechnik.com
Signed-off-by: Paul Hollinsky <phollinsky@holtechnik.com>
---
Reported and analysed in:
https://lore.kernel.org/linux-arm-msm/20260808034716.58888-1-phollinsky@holtechnik.com/
Konrad agreed with this shape in
https://lore.kernel.org/linux-arm-msm/b63e93e4-2f4c-4cad-b726-e1b0565379d7@gmail.com/

0be72be03ca7 landed in mainline during the v7.3 merge window (via the
soc-drivers-7.3 pull), so this is based on Linus' tree. Note that
msm-fixes and msm-next both still predate it as I write this, so it
needs a base that includes the merge window; happy to respin against
whatever base you prefer, and it can equally go via the qcom tree the
culprit came through.

Tested on a Lenovo IdeaPad Duet 3 (sc7180-trogdor-wormdingler) on
next-20260805, which carries the same code: with this applied the GPU
initialises and logs "Zap shader not enabled - using SECVID_TRUST_CNTL
instead", and userspace gets a6xx rather than llvmpipe.

 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 489462065ea9..995161215ad5 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -52,6 +52,12 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
 		return -ENODEV;
 	}
 
+	/* We need PAS to be able to load the firmware */
+	if (!qcom_pas_is_available()) {
+		DRM_DEV_ERROR(dev, "PAS is not available\n");
+		return -EPROBE_DEFER;
+	}
+
 	ret = of_reserved_mem_region_to_resource(np, 0, &r);
 	if (ret) {
 		zap_available = false;
@@ -170,18 +176,11 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
 int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
-	struct platform_device *pdev = gpu->pdev;
 
 	/* Short cut if we determine the zap shader isn't available/needed */
 	if (!zap_available)
 		return -ENODEV;
 
-	/* 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;
-	}
-
 	return zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw, pasid);
 }
 

base-commit: 818bebeb63dd6bf5f4e07e145f6cdbace520a34c
-- 
2.55.0

Re: [PATCH] drm/msm/adreno: Only check for PAS when a zap shader is present

[sashiko-bot]

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

Pre-existing issues:
- [High] The device node 'np' acquired via `of_get_available_child_by_name()` is never released with `of_node_put()`, causing a reference count leak. This patch makes the leak repeatedly reachable from userspace.
- [High] Unchecked return value from `kasprintf()` leads to a NULL pointer dereference and kernel panic.
--

--- Patch [1]: [PATCH] drm/msm/adreno: Only check for PAS when a zap shader is present ---
commit 84273d207b58c12797426ab6b4931f54563e7fed
Author: Paul Hollinsky <phollinsky@holtechnik.com>

drm/msm/adreno: Only check for PAS when a zap shader is present

This commit moves the qcom_pas_is_available() check from
adreno_zap_shader_load() into zap_shader_load_mdt() after checking for the
presence of a zap-shader node. This prevents a permanent -EPROBE_DEFER on
boards that lack PAS and don't need a zap shader.

Link: https://lore.kernel.org/r/20260808034716.58888-1-phollinsky@holtechnik.com

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 489462065ea95..995161215ad56 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -52,6 +52,12 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
>  		return -ENODEV;
>  	}
>  
> +	/* We need PAS to be able to load the firmware */
> +	if (!qcom_pas_is_available()) {
> +		DRM_DEV_ERROR(dev, "PAS is not available\n");
> +		return -EPROBE_DEFER;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this new error return path leak the
device node reference for np?

of_get_available_child_by_name() increments the reference count of the
returned device node, which needs to be released with of_node_put().

It appears zap_shader_load_mdt() completely lacks any of_node_put() calls
on its exit paths. Because this patch moves the qcom_pas_is_available()
check after the node lookup, a failure here returns -EPROBE_DEFER and leaves
the node refcounted.

As noted in the commit message, since GPU initialization is re-attempted on
every DRM open() syscall, could this allow an unprivileged user to
continuously leak the device node reference count from userspace?

[ ... ]

While reviewing this patch, I also noticed another pre-existing issue in
zap_shader_load_mdt() further down in the file:

[Severity: High]
This is a pre-existing issue, but does this code lead to a NULL pointer
dereference if the memory allocation fails?

drivers/gpu/drm/msm/adreno/adreno_gpu.c:zap_shader_load_mdt() {
...
		char *newname;

		newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname);

		ret = qcom_mdt_load(dev, fw, newname, pasid,
				mem_region, mem_phys, mem_size, NULL);
		kfree(newname);
...
}

kasprintf() is called with GFP_KERNEL and can return NULL if the memory
allocation fails.

The resulting newname pointer is passed immediately as the fw_name argument
to qcom_mdt_load() without a NULL check.

Since the pointer is passed down the call chain:
qcom_mdt_load() -> qcom_mdt_load_no_init() -> qcom_mdt_read_metadata() ->
mdt_load_split_segment() -> strlen(fw_name)

Will passing NULL to strlen() cause a kernel panic here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821081325.89088-1-phollinsky@holtechnik.com?part=1