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 <[email protected]> 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/[email protected]?part=1
