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

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 <[email protected]>

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/[email protected]

> 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/[email protected]?part=1