Re: [PATCH v2 3/9] drm/msm/adreno: migrate zap shader loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/msm/adreno: missing qcom_pas_metadata_release() in
  zap_shader_load_mdt()
--

commit 8918df651bb53ba4326a0ffac4f3a572875b140d
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

drm/msm/adreno: migrate zap shader loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

This commit updates zap_shader_load_mdt() to load and authenticate the zap
shader using qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset() with a
stack-allocated struct qcom_pas_context.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index dadfe4c694384..12e35841763d8 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -34,11 +34,11 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
>  	struct device *dev = &gpu->pdev->dev;
>  	const struct firmware *fw;
>  	const char *signed_fwname = NULL;
> +	struct qcom_pas_context ctx;
>  	struct device_node *np;
>  	struct resource r;
>  	phys_addr_t mem_phys;
>  	ssize_t mem_size;
> -	void *mem_region = NULL;
>  	int ret;
>  
>  	if (!IS_ENABLED(CONFIG_ARCH_QCOM)) {
> @@ -121,39 +121,32 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
>  		goto out;
>  	}
>  
> -	/* Allocate memory for the firmware image */
> -	mem_region = memremap(mem_phys, mem_size,  MEMREMAP_WC);
> -	if (!mem_region) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	qcom_pas_context_init(&ctx, dev, pasid, mem_phys, mem_size);
>  
>  	/*
>  	 * Load the rest of the MDT
>  	 *
>  	 * Note that we could be dealing with two different paths, since
>  	 * with upstream linux-firmware it would be in a qcom/ subdir..
> -	 * adreno_request_fw() handles this, but qcom_mdt_load() does
> +	 * adreno_request_fw() handles this, but qcom_mdt_pas_load() does
>  	 * not.  But since we've already gotten through adreno_request_fw()
>  	 * we know which of the two cases it is:
>  	 */
>  	if (signed_fwname || (to_adreno_gpu(gpu)->fwloc == FW_LOCATION_LEGACY)) {
> -		ret = qcom_mdt_load(dev, fw, fwname, pasid,
> -				mem_region, mem_phys, mem_size, NULL);
> +		ret = qcom_mdt_pas_load(&ctx, fw, fwname, NULL);
>  	} else {
>  		char *newname;
>  
>  		newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname);
>  
> -		ret = qcom_mdt_load(dev, fw, newname, pasid,
> -				mem_region, mem_phys, mem_size, NULL);
> +		ret = qcom_mdt_pas_load(&ctx, fw, newname, NULL);
>  		kfree(newname);
>  	}
>  	if (ret)
>  		goto out;
>  
>  	/* Send the image to the secure world */
> -	ret = qcom_pas_auth_and_reset(pasid);
> +	ret = qcom_pas_prepare_and_auth_reset(&ctx);
>  
>  	/*
>  	 * If the pas call returns -EOPNOTSUPP we assume that this target
> @@ -165,9 +158,6 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
>  		DRM_DEV_ERROR(dev, "Unable to authorize the image\n");
>  
>  out:
> -	if (mem_region)
> -		memunmap(mem_region);
> -
>  	release_firmware(fw);
>  
>  	return ret;

[Severity: High]
Does this leak the PAS metadata buffer stored in ctx.ptr?

Previously, qcom_mdt_load() passed a NULL PAS context to
__qcom_mdt_pas_init(), so qcom_pas_init_image() freed the metadata buffer
immediately.  Passing &ctx to qcom_mdt_pas_load() causes
qcom_pas_init_image() to retain the allocated metadata buffer in ctx.ptr,
and zap_shader_load_mdt() returns without calling
qcom_pas_metadata_release(&ctx).

Commit 2aa8fafc08e7 ("firmware: qcom: pas: Make MDT buffer retention
explicit") changes qcom_pas_init_image() to release metadata unless
ctx->keep_mdt_buf is set, but this series is based on a tree without that
commit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113185009.157893-1-mukesh.ojha@oss.qualcomm.com?part=3