Re: [PATCH v2 5/9] media: venus: migrate firmware 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:
- [Medium] media: venus: PAS metadata buffer leak in venus_load_fw() and
  venus_boot()
--

commit 82fbd4dcc81174e35cd1220225cd23273f0e119b
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

media: venus: migrate firmware loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

This commit migrates the TrustZone firmware loading path in the Venus
driver to use a PAS context with qcom_mdt_pas_load() and
qcom_pas_prepare_and_auth_reset().

> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 3a38ff985822f..704343e241dc6 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -117,21 +117,22 @@ static int venus_load_fw(struct venus_core *core, const char *fwname,
>  		goto err_release_fw;
>  	}
>  
> -	mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
> -	if (!mem_va) {
> -		dev_err(dev, "unable to map memory region %pa size %#zx\n", mem_phys, *mem_size);
> -		ret = -ENOMEM;
> -		goto err_release_fw;
> -	}
> -
> -	if (core->use_tz)
> -		ret = qcom_mdt_load(dev, mdt, fwname, VENUS_PAS_ID,
> -				    mem_va, *mem_phys, *mem_size, NULL);
> -	else
> +	if (core->use_tz) {
> +		qcom_pas_context_init(&core->fw.pas_ctx, dev, VENUS_PAS_ID,
> +				      *mem_phys, *mem_size);
> +		ret = qcom_mdt_pas_load(&core->fw.pas_ctx, mdt, fwname, NULL);
> +	} else {
> +		mem_va = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
> +		if (!mem_va) {
> +			dev_err(dev, "unable to map memory region %pa size %#zx\n",
> +				mem_phys, *mem_size);
> +			ret = -ENOMEM;
> +			goto err_release_fw;
> +		}
>  		ret = qcom_mdt_load_no_init(dev, mdt, fwname, mem_va,
>  					    *mem_phys, *mem_size, NULL);
> -
> -	memunmap(mem_va);
> +		memunmap(mem_va);
> +	}
>  err_release_fw:
>  	release_firmware(mdt);
>  	return ret;
> @@ -237,7 +238,7 @@ int venus_boot(struct venus_core *core)
>  	core->fw.mem_phys = mem_phys;
>  
>  	if (core->use_tz)
> -		ret = qcom_pas_auth_and_reset(VENUS_PAS_ID);
> +		ret = qcom_pas_prepare_and_auth_reset(&core->fw.pas_ctx);
>  	else
>  		ret = venus_boot_no_tz(core, mem_phys, mem_size);

[Severity: Medium]
As reported by Konrad Dybcio on v1 [1], does this leak the PAS metadata
buffer stored in core->fw.pas_ctx.ptr?

Unlike qcom_mdt_load(), which passes a NULL context to
qcom_pas_init_image() so the metadata buffer is freed immediately,
qcom_mdt_pas_load() passes &core->fw.pas_ctx and retains the metadata
allocation in core->fw.pas_ctx.ptr until qcom_pas_metadata_release() is
called.  Because qcom_pas_metadata_release(&core->fw.pas_ctx) is never
called after qcom_pas_prepare_and_auth_reset() or on error in
qcom_mdt_pas_load(), every boot and system error recovery cycle in
venus_sys_error_handler() -> venus_boot() overwrites core->fw.pas_ctx via
qcom_pas_context_init() and leaks the buffer.

[1] https://lore.kernel.org/r/ce3f58f7-f500-49f8-89cd-611d8e1ac6ac@oss.qualcomm.com

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