Re: [PATCH] drm/amdgpu/discovery: fix OOB read via unchecked die_offset in IP discovery parsing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch identifies but fails to fix out-of-bounds reads caused by unbounded ip_offset advancement.
- [High] Missing bounds check for firmware-controlled num_dies before accessing the die_info array.
- [High] Missing bounds checks for basic header offsets in the discovery binary parser.
--

commit 9d6788c99594a0d1857e6cde0b70c40338a5e49f
Author: Pavitra Jha <jhapavitra98@gmail.com>

drm/amdgpu/discovery: fix OOB read via unchecked die_offset in IP discovery parsing

This commit adds bounds checking for die_offset against adev->discovery.size
in three IP discovery parsing functions. It aims to prevent out-of-bounds
reads when processing firmware-controlled die_offset values.

> The ip_offset advancement inside the inner loop also uses
> struct_size(ip, base_address, ip->num_base_address) where
> num_base_address is firmware-controlled, enabling unbounded advancement
> past the allocation on each iteration.

[Severity: High]
This is a pre-existing issue, but the commit message identifies this unbounded
advancement as a root cause, yet the patch doesn't seem to include a bounds
check for it in the loop.

Should there be a check against adev->discovery.size for ip_offset inside the
inner loop as well, to prevent reading past the end of the allocation when
dereferencing the advanced ip_offset?


> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index be5069642a907..41ca01e2bb6fd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -774,6 +774,11 @@ static void amdgpu_discovery_read_harvest_bit_per_ip(struct amdgpu_device *adev,
>  	/* scan harvest bit of all IP data structures */

[Severity: High]
This is a pre-existing issue, but does the construction of the ihdr pointer
lack bounds checking?

In amdgpu_discovery_read_harvest_bit_per_ip(), ihdr is constructed using an
unchecked offset:

ihdr = (struct ip_discovery_header *)(discovery_bin +
             le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset));

Could an invalid firmware-provided offset cause an out-of-bounds read when
ihdr is later dereferenced?

>  	for (i = 0; i < num_dies; i++) {
>  		die_offset = le16_to_cpu(ihdr->die_info[i].die_offset);

[Severity: High]
This is a pre-existing issue, but is it possible for num_dies to exceed the
fixed array size of 16 for die_info?

Since num_dies is firmware-controlled, could an unchecked value cause an
out-of-bounds read when accessing ihdr->die_info[i]?

> +		if (die_offset + sizeof(*dhdr) > adev->discovery.size) {
> +			dev_err(adev->dev, "invalid die_offset %u in harvest table\n",
> +				die_offset);
> +			return;
> +		}
>  		dhdr = (struct die_header *)(discovery_bin + die_offset);
>  		num_ips = le16_to_cpu(dhdr->num_ips);
>  		ip_offset = die_offset + sizeof(*dhdr);

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