Re: [PATCH] drm/amdgpu/gfx8: fix register_list_format leak in gfx_v8_0_init_microcode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing bounds checking and potential integer overflow in firmware header parsing leading to a heap buffer overflow.
--

commit 85ca74ba24e5f9c96f4d94f3bcb428aa05e58c7a
Author: Willian Oliveira <williandossantosdeoliveira287@gmail.com>

drm/amdgpu/gfx8: fix register_list_format leak in gfx_v8_0_init_microcode
    
gfx_v8_0_init_microcode() allocates rlc.register_list_format before
loading the MEC firmware.

If loading one of the subsequent firmware files fails, the function
returns through the error path without freeing register_list_format.

Use gfx_v8_0_free_microcode() in the error path so all previously
allocated firmware resources, including register_list_format, are
released.

> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 6cf4279950787..92c56d811975c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -1211,12 +1211,7 @@ static int gfx_v8_0_init_microcode(struct amdgpu_device *adev)
>  out:
>  	if (err) {
>  		dev_err(adev->dev, "gfx8: Failed to load firmware %s gfx firmware\n", chip_name);
> -		amdgpu_ucode_release(&adev->gfx.pfp_fw);
> -		amdgpu_ucode_release(&adev->gfx.me_fw);
> -		amdgpu_ucode_release(&adev->gfx.ce_fw);
> -		amdgpu_ucode_release(&adev->gfx.rlc_fw);
> -		amdgpu_ucode_release(&adev->gfx.mec_fw);
> -		amdgpu_ucode_release(&adev->gfx.mec2_fw);
> +		gfx_v8_0_free_microcode(adev);

[Severity: Critical]
This is a pre-existing issue, but while reviewing this function's error
handling, I noticed a potential integer overflow in the allocation of
register_list_format higher up in gfx_v8_0_init_microcode():

	adev->gfx.rlc.reg_list_format_size_bytes =
			le32_to_cpu(rlc_hdr->reg_list_format_size_bytes);
	adev->gfx.rlc.reg_list_size_bytes =
			le32_to_cpu(rlc_hdr->reg_list_size_bytes);

	adev->gfx.rlc.register_list_format =
			kmalloc(adev->gfx.rlc.reg_list_format_size_bytes +
					adev->gfx.rlc.reg_list_size_bytes, GFP_KERNEL);

Are we sure reg_list_format_size_bytes and reg_list_size_bytes can't overflow
when added together?

If a maliciously crafted RLC firmware provides large values for these two
variables, the 32-bit addition could wrap around, resulting in a very small
allocation.

Because the subsequent loops iterate over the original un-overflowed values
like this:

	for (i = 0 ; i < (adev->gfx.rlc.reg_list_format_size_bytes >> 2); i++)
		adev->gfx.rlc.register_list_format[i] = le32_to_cpu(tmp[i]);

could this cause out-of-bounds writes into the undersized heap buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905133405.3733-1-williandossantosdeoliveira287@gmail.com?part=1