media: iris: Fix firmware reference leak and unmap memory after load

[PATCH] media: iris: Fix firmware reference leak and unmap memory after load

Posted by Stephan Gerhold 1 month, 2 weeks ago

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@vger.kernel.org
Fixes: d19b163356b8 ("media: iris: implement video firmware load/unload")
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.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,
-- 
Stephan Gerhold <stephan.gerhold@linaro.org>

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

Posted by Dikshita Agarwal 1 month, 1 week ago

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@vger.kernel.org
> Fixes: d19b163356b8 ("media: iris: implement video firmware load/unload")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.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@quicinc.com>

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

Posted by Stephan Gerhold 1 month, 1 week ago

On Thu, Aug 21, 2025 at 02:33:18PM +0530, Dikshita Agarwal wrote:
> 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.

Yeah, I had a bit weird choice between To/Cc in this patch. I'll change
it for my patches in the future.

Note however that there isn't any convention given for To/Cc in the
process document [1]. It just says you should "copy the appropriate
subsystem maintainer(s)", so everyone does it a bit differently.

I would recommend to avoid relying on To vs Cc, to make sure you don't
accidentally miss submissions from others as well.

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

Thanks for the review!

Stephan

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

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

Posted by Bryan O'Donoghue 1 month, 2 weeks ago

On 18/08/2025 10:50, Stephan Gerhold wrote:
> +	ret = qcom_scm_pas_auth_and_reset(core->iris_platform_data->pas_id);

You're not using the latched pas_id declared @ the top of this function.

With that fixed.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

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

Posted by Bryan O'Donoghue 1 month, 2 weeks ago

On 18/08/2025 12:01, Bryan O'Donoghue wrote:
> On 18/08/2025 10:50, Stephan Gerhold wrote:
>> +    ret = qcom_scm_pas_auth_and_reset(core->iris_platform_data->pas_id);
> 
> You're not using the latched pas_id declared @ the top of this function.
> 
> With that fixed.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

No I'm wrong you've moved this to a separate function.

This patch is fine as-is.

---
bod