Re: [PATCH v2] drm/amdgpu/discovery: validate table offset before IP discovery header cast

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Misapplied patch hunk causes compilation errors.

Pre-existing issues:
- [High] The intended security fix is completely missing from `amdgpu_discovery_get_table_info`, leaving out-of-bounds reads unpatched.
- [High] The newly added offset validation only checks the start offset, failing to verify that the structure fits within the buffer.
- [High] Missing validation for downstream table sizes and internal offsets causes out-of-bounds reads during parsing and checksum verification.
- [High] Direct array access for IP_DISCOVERY table offsets breaks for version 2 binaries.
--

--- Patch [1]: [PATCH v2] drm/amdgpu/discovery: validate table offset before IP discovery header cast ---
commit 9aac939f5a3fe8366dddb94f326456646c35f228
Author: Pavitra Jha <jhapavitra98@gmail.com>

drm/amdgpu/discovery: validate table offset before IP discovery header cast

The commit intends to validate the firmware-controlled table offset before
casting it to the IP discovery header to prevent out-of-bounds reads. It
aims to add offset checks in both the common parsing path and direct access
sites.

> Fix by validating the table offset in amdgpu_discovery_get_table_info(),
> which is the common path used by all callers except
> amdgpu_discovery_read_harvest_bit_per_ip() (which reads

[Severity: High]
This isn't a bug introduced by this patch, but the commit message states the
fix is added to amdgpu_discovery_get_table_info(), while the diff actually
applies the validation hunk to amdgpu_discovery_verify_npsinfo() instead. 

Since amdgpu_discovery_get_table_info() remains unpatched, doesn't this leave
the shared table parsing paths vulnerable to out-of-bounds reads when parsing
firmware data?

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index a015d55aa158e..07544bcfaf2ef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -531,6 +531,12 @@ static int amdgpu_discovery_verify_npsinfo(struct amdgpu_device *adev,
>  		return -EINVAL;
>  	}
>  
> +	if (le16_to_cpu((*info)->offset) >= adev->discovery.size) {
> +		dev_err(adev->dev, "invalid table offset %u for table_id %u\n",
> +			le16_to_cpu((*info)->offset), table_id);
> +		return -EINVAL;
> +	}
> +

[Severity: Low]
Does this compile? 

In amdgpu_discovery_verify_npsinfo(), info is a single pointer of type struct
table_info *info, making (*info)->offset invalid C syntax. Additionally,
table_id is undeclared in this function. It appears this hunk was intended
for amdgpu_discovery_get_table_info() but was misapplied here.

[ ... ]
> @@ -803,6 +809,14 @@ static void amdgpu_discovery_read_harvest_bit_per_ip(struct amdgpu_device *adev,
>  	int i, j;
>  
>  	bhdr = (struct binary_header *)discovery_bin;

[Severity: High]
This is a pre-existing issue, but does unconditionally casting to
struct binary_header * work for version 2 binaries? 

Since version 2 binaries (struct binary_header_v2) insert num_tables and
padding before the table_list array, wouldn't accessing
bhdr->table_list[IP_DISCOVERY].offset read from those fields instead,
leading to incorrect offset calculations and out-of-bounds accesses?

> +
> +	if (le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset) >=
> +	    adev->discovery.size) {

[Severity: High]
This is a pre-existing issue, but does this check adequately prevent
out-of-bounds reads? 

The check only verifies that the start offset is within the buffer bounds. If
an offset is placed at the very end of the buffer, wouldn't the subsequent
read of ihdr->num_dies access memory beyond the buffer since the structure
size is not validated?

> +		dev_err(adev->dev, "invalid IP_DISCOVERY table offset %u\n",
> +			le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset));
> +		return;
> +	}
> +
>  	ihdr = (struct ip_discovery_header
>  			*)(discovery_bin +
>  			   le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset));

[Severity: High]
This is a pre-existing issue, but are downstream table sizes and internal
offsets validated to prevent out-of-bounds reads?

For example, in amdgpu_discovery_read_harvest_bit_per_ip(), ip_offset is
declared as a uint16_t:

	ip_offset += struct_size(ip, base_address,
				 ip->num_base_address);

Could a struct_size() overflow result in SIZE_MAX being truncated to a
smaller value by uint16_t, thereby defeating built-in overflow protections?

Additionally, in amdgpu_discovery_init(), is it possible for the size
calculation to underflow?

	size = le16_to_cpu(bhdr->binary_size) - offset;

If binary_size is less than offset, this underflow could lead to a large
out-of-bounds read in the checksum verification loop. 

Similarly, in amdgpu_discovery_table_check(), table_size is read from headers
and passed directly to amdgpu_discovery_verify_checksum(). Is there a check
to ensure it fits within the buffer?

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