linux-kernel - Re: [PATCH] media: iris: Fix firmware reference leak and unmap memory after load

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe1fe768-678a-48db-c603-2fda3effffb9@quicinc.com>
Date: Thu, 21 Aug 2025 14:33:18 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Stephan Gerhold <stephan.gerhold@...aro.org>,
        Bryan O'Donoghue
	<bryan.odonoghue@...aro.org>
CC: Vikash Garodia <quic_vgarodia@...cinc.com>,
        Abhinav Kumar
	<abhinav.kumar@...ux.dev>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Stefan Schmidt" <stefan.schmidt@...aro.org>,
        Hans Verkuil
	<hverkuil@...nel.org>, <linux-media@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Neil
 Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH] media: iris: Fix firmware reference leak and unmap memory
 after load

Hi Stephan,

I noticed that the maintainers were included in the CC list rather than the
"To" field. please ensure that all relevant maintainers are added directly
to the "To" list in your future submissions.

On 8/18/2025 3:20 PM, Stephan Gerhold wrote:
> When we succeed loading the firmware, we don't want to hold on to the
> firmware pointer anymore, since it won't be freed anywhere else. The same
> applies for the mapped memory. Unmapping the memory is particularly
> important since the memory will be protected after the Iris firmware is
> started, so we need to make sure there will be no accidental access to this
> region (even if just a speculative one from the CPU).
> 
> Almost the same firmware loading code also exists in venus/firmware.c,
> there it is implemented correctly.
> 
> Fix this by dropping the early "return ret" and move the call of
> qcom_scm_pas_auth_and_reset() out of iris_load_fw_to_memory(). We should
> unmap the memory before bringing the firmware out of reset.
> 
> Cc: stable@...r.kernel.org
> Fixes: d19b163356b8 ("media: iris: implement video firmware load/unload")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@...aro.org>
> ---
>  drivers/media/platform/qcom/iris/iris_firmware.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> index f1b5cd56db3225d0a97e07d3a63c24814deeba78..9ab499fad946446a87036720f49c9c8d311f3060 100644
> --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> @@ -60,16 +60,7 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
>  
>  	ret = qcom_mdt_load(dev, firmware, fw_name,
>  			    pas_id, mem_virt, mem_phys, res_size, NULL);
> -	if (ret)
> -		goto err_mem_unmap;
> -
> -	ret = qcom_scm_pas_auth_and_reset(pas_id);
> -	if (ret)
> -		goto err_mem_unmap;
> -
> -	return ret;
>  
> -err_mem_unmap:
>  	memunmap(mem_virt);
>  err_release_fw:
>  	release_firmware(firmware);
> @@ -94,6 +85,12 @@ int iris_fw_load(struct iris_core *core)
>  		return -ENOMEM;
>  	}
>  
> +	ret = qcom_scm_pas_auth_and_reset(core->iris_platform_data->pas_id);
> +	if (ret)  {
> +		dev_err(core->dev, "auth and reset failed: %d\n", ret);
> +		return ret;
> +	}
> +
>  	ret = qcom_scm_mem_protect_video_var(cp_config->cp_start,
>  					     cp_config->cp_size,
>  					     cp_config->cp_nonpixel_start,
> 
> ---
> base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
> change-id: 20250815-iris-firmware-leak-b6c43bd1ee85
> 
> Best regards,

Reviewed-by: Dikshita Agarwal <quic_dikshita@...cinc.com>