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

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

[Pavitra Jha]

Three call sites in amdgpu_discovery.c dereference firmware-controlled
die_offset values without validating them against adev->discovery.size:

  amdgpu_discovery_read_harvest_bit_per_ip() line 776-777
  amdgpu_discovery_sysfs_init()              line 1298-1299
  amdgpu_discovery_reg_base_init()           line 1524-1525

In all three sites the pattern is:

  die_offset = le16_to_cpu(ihdr->die_info[i].die_offset);
  dhdr = (struct die_header *)(discovery_bin + die_offset);

die_offset is a firmware-controlled u16 (max 65535). The discovery
binary is allocated as adev->discovery.size bytes (DISCOVERY_TMR_SIZE
= 10240 by default). No bounds check exists between the le16_to_cpu()
call and the pointer cast, so a crafted blob with die_offset >= 10240
produces a pointer past the end of the allocation. The subsequent reads
of dhdr->die_id and dhdr->num_ips are then slab-out-of-bounds reads.

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.

ASAN report (kernel 7.1.0+, QEMU/x86_64, nokaslr, slub_debug=FZPUA):

==================================================================
BUG: KASAN: slab-out-of-bounds in poc_init+0x453/0x1000 [amd_oob_harness]
Read of size 2 at addr ffff88800318a801 by task insmod/22

CPU: 0 UID: 0 PID: 22 Comm: insmod Tainted: G           O        7.1.0+ #26 PREEMPTLAZY
Tainted: [O]=OOT_MODULE
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-debian-1.17.0-1 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x2b/0x40
 print_report+0x14f/0x4d0
 ? wake_up_klogd_work_func+0x70/0x70
 ? poc_exit+0xfc0/0xfc0 [amd_oob_harness]
 kasan_report+0xd4/0x100
 ? poc_init+0x453/0x1000 [amd_oob_harness]
 ? poc_init+0x453/0x1000 [amd_oob_harness]
 poc_init+0x453/0x1000 [amd_oob_harness]
 ? poc_exit+0xfc0/0xfc0 [amd_oob_harness]
 ? poc_exit+0xfc0/0xfc0 [amd_oob_harness]
 do_one_initcall+0xb0/0x230
 ? initcall_blacklisted+0x150/0x150
 ? kasan_unpoison+0x40/0x60
 do_init_module+0x263/0x810
 ? kasan_save_free_info+0x37/0x50
 ? free_module+0x300/0x300
 ? kfree+0xf1/0x390
 load_module+0x3e12/0x51e0
 ? sysvec_apic_timer_interrupt+0xa/0x80
 ? asm_sysvec_apic_timer_interrupt+0x16/0x20
 ? module_frob_arch_sections+0x20/0x20
 ? kernel_read_file+0x4d9/0x790
 ? kernel_read_file+0x36c/0x790
 init_module_from_file+0x136/0x150
 ? __do_sys_init_module+0x180/0x180
 ? do_sys_openat2+0xeb/0x140
 ? fdget+0x64/0x200
 __x64_sys_finit_module+0x39f/0x7a0
 ? __x64_sys_init_module+0xc0/0xc0
 ? __x64_sys_open+0x180/0x180
 do_syscall_64+0x56/0x3f0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53
RIP: 0033:0x4d1259
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fffb68052f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000004d1259
RDX: 0000000000000000 RSI: 000000000a529cf0 RDI: 0000000000000003
RBP: 000000000a529cf0 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000a528dd0
R13: 0000000000000003 R14: 0000000000000000 R15: 0000000000000000
 </TASK>

The buggy address belongs to the physical page:
page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x3188
head: order:2 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0x4000000000000040(head|zone=1)
page_type: f8(unknown)
raw: 4000000000000040 0000000000000000 dead000000000122 0000000000000000
raw: 0000000000000000 0000000000000000 00000000f8000000 0000000000000000
head: 4000000000000040 0000000000000000 dead000000000122 0000000000000000
head: 0000000000000000 0000000000000000 00000000f8000000 0000000000000000
head: 4000000000000002 ffffea00000c6201 00000000ffffffff 00000000ffffffff
head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88800318a700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff88800318a780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff88800318a800: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
                   ^
 ffff88800318a880: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
 ffff88800318a900: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
==================================================================
BUG: KASAN: slab-out-of-bounds in poc_init+0x4b3/0x1000 [amd_oob_harness]
Read of size 2 at addr ffff88800318a803 by task insmod/22

CPU: 0 UID: 0 PID: 22 Comm: insmod Tainted: G    B      O        7.1.0+ #26 PREEMPTLAZY
Tainted: [B]=BAD_PAGE, [O]=OOT_MODULE
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-debian-1.17.0-1 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x2b/0x40
 print_report+0x14f/0x4d0
 ? add_taint+0x50/0x70
 kasan_report+0xd4/0x100
 ? poc_init+0x4b3/0x1000 [amd_oob_harness]
 ? poc_init+0x4b3/0x1000 [amd_oob_harness]
 poc_init+0x4b3/0x1000 [amd_oob_harness]
 ? poc_exit+0xfc0/0xfc0 [amd_oob_harness]
 ? poc_exit+0xfc0/0xfc0 [amd_oob_harness]
 do_one_initcall+0xb0/0x230
 ? initcall_blacklisted+0x150/0x150
 ? kasan_unpoison+0x40/0x60
 do_init_module+0x263/0x810
 ? kasan_save_free_info+0x37/0x50
 ? free_module+0x300/0x300
 ? kfree+0xf1/0x390
 load_module+0x3e12/0x51e0
 ? sysvec_apic_timer_interrupt+0xa/0x80
 ? asm_sysvec_apic_timer_interrupt+0x16/0x20
 ? module_frob_arch_sections+0x20/0x20
 ? kernel_read_file+0x4d9/0x790
 ? kernel_read_file+0x36c/0x790
 init_module_from_file+0x136/0x150
 ? __do_sys_init_module+0x180/0x180
 ? do_sys_openat2+0xeb/0x140
 ? fdget+0x64/0x200
 __x64_sys_finit_module+0x39f/0x7a0
 ? __x64_sys_init_module+0xc0/0xc0
 ? __x64_sys_open+0x180/0x180
 do_syscall_64+0x56/0x3f0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53
RIP: 0033:0x4d1259
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fffb68052f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000007ffff0 RDI: 0000000000000003
RBP: 000000000a529cf0 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000a528dd0
R13: 0000000000000003 R14: 0000000000000000 R15: 0000000000000000
 </TASK>

The buggy address belongs to the physical page:
page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x3188
head: order:2 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
flags: 0x4000000000000040(head|zone=1)
page_type: f8(unknown)
raw: 4000000000000040 0000000000000000 dead000000000122 0000000000000000
raw: 0000000000000000 0000000000000000 00000000f8000000 0000000000000000
head: 4000000000000040 0000000000000000 dead000000000122 0000000000000000
head: 0000000000000000 0000000000000000 00000000f8000000 0000000000000000
head: 4000000000000002 ffffea00000c6201 00000000ffffffff 00000000ffffffff
head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88800318a700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 ffff88800318a780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffff88800318a800: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
                   ^
 ffff88800318a880: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
 ffff88800318a900: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
==================================================================

Fix by adding a bounds check on die_offset against adev->discovery.size
before the pointer cast in all three sites.

Fixes: d0c647a6aae2 ("drm/amdgpu/discovery: support new discovery binary header")
Cc: stable@vger.kernel.org
Signed-off-by: Pavitra Jha <jhapavitra98@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index be5069642..41ca01e2b 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 */
 	for (i = 0; i < num_dies; i++) {
 		die_offset = le16_to_cpu(ihdr->die_info[i].die_offset);
+		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);
@@ -1296,6 +1301,11 @@ static int amdgpu_discovery_sysfs_recurse(struct amdgpu_device *adev)
 		struct ip_die_entry *ip_die_entry;
 
 		die_offset = le16_to_cpu(ihdr->die_info[ii].die_offset);
+		if (die_offset + sizeof(*dhdr) > adev->discovery.size) {
+			dev_err(adev->dev, "invalid die_offset %u in sysfs init\n",
+				die_offset);
+			return -EINVAL;
+		}
 		dhdr = (struct die_header *)(discovery_bin + die_offset);
 		num_ips = le16_to_cpu(dhdr->num_ips);
 		ip_offset = die_offset + sizeof(*dhdr);
@@ -1522,6 +1532,11 @@ static int amdgpu_discovery_reg_base_init(struct amdgpu_device *adev)
 
 	for (i = 0; i < num_dies; i++) {
 		die_offset = le16_to_cpu(ihdr->die_info[i].die_offset);
+		if (die_offset + sizeof(*dhdr) > adev->discovery.size) {
+			dev_err(adev->dev, "invalid die_offset %u in reg base init\n",
+				die_offset);
+			return -EINVAL;
+		}
 		dhdr = (struct die_header *)(discovery_bin + die_offset);
 		num_ips = le16_to_cpu(dhdr->num_ips);
 		ip_offset = die_offset + sizeof(*dhdr);
-- 
2.53.0

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

[sashiko-bot]

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

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

[Pavitra Jha]

Sashiko AI review of the previous fix flagged three remaining gaps in
the discovery blob parser, all stemming from the same root cause: the
ip_discovery_header pointer itself is constructed from a firmware-
controlled offset with no validation before the cast.

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

This offset is a firmware-controlled u16 read directly from the
discovery blob's table_list, with no bounds check against
adev->discovery.size before being used to construct ihdr. Every
subsequent read from ihdr, including num_dies and die_info[], is
downstream of this unchecked pointer.

The other two items in that review (unbounded ip_offset advancement
via num_base_address, and num_dies exceeding die_info[]'s capacity)
were already addressed in the previous fix.

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
table_list[IP_DISCOVERY].offset directly rather than going through
get_table_info()). Add the equivalent check at that direct access site
as well, so all paths that construct an ip_discovery_header pointer
from a table offset are covered.

The check validates the offset itself against adev->discovery.size,
independent of any specific downstream struct size, since
get_table_info() is shared across ten different table types
(IP_DISCOVERY, HARVEST_INFO, GC, MALL_INFO, VCN_INFO, NPS_INFO, and
others) each with differently-sized table structures.

Fixes: d0c647a6aae2 ("drm/amdgpu/discovery: support new discovery binary header")
Cc: stable@vger.kernel.org
Signed-off-by: Pavitra Jha <jhapavitra98@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index b4ee5fc8e..9b55c56cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -564,6 +564,12 @@ static int amdgpu_discovery_get_table_info(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;
+	}
+
 	return 0;
 }
 
@@ -766,6 +772,14 @@ static void amdgpu_discovery_read_harvest_bit_per_ip(struct amdgpu_device *adev,
 	int i, j;
 
 	bhdr = (struct binary_header *)discovery_bin;
+
+	if (le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset) >=
+	    adev->discovery.size) {
+		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));
-- 
2.53.0

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

[sashiko-bot]

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

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

[Alex Deucher]

[-- Attachment #1: Type: text/plain, Size: 3685 bytes --]

On Wed, Jul 8, 2026 at 9:19 AM Pavitra Jha <jhapavitra98@gmail.com> wrote:
>
> Sashiko AI review of the previous fix flagged three remaining gaps in
> the discovery blob parser, all stemming from the same root cause: the
> ip_discovery_header pointer itself is constructed from a firmware-
> controlled offset with no validation before the cast.
>
>   ihdr = (struct ip_discovery_header *)(discovery_bin +
>                     le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset));
>
> This offset is a firmware-controlled u16 read directly from the
> discovery blob's table_list, with no bounds check against
> adev->discovery.size before being used to construct ihdr. Every
> subsequent read from ihdr, including num_dies and die_info[], is
> downstream of this unchecked pointer.
>
> The other two items in that review (unbounded ip_offset advancement
> via num_base_address, and num_dies exceeding die_info[]'s capacity)
> were already addressed in the previous fix.
>
> 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
> table_list[IP_DISCOVERY].offset directly rather than going through
> get_table_info()). Add the equivalent check at that direct access site
> as well, so all paths that construct an ip_discovery_header pointer
> from a table offset are covered.
>
> The check validates the offset itself against adev->discovery.size,
> independent of any specific downstream struct size, since
> get_table_info() is shared across ten different table types
> (IP_DISCOVERY, HARVEST_INFO, GC, MALL_INFO, VCN_INFO, NPS_INFO, and
> others) each with differently-sized table structures.
>
> Fixes: d0c647a6aae2 ("drm/amdgpu/discovery: support new discovery binary header")
> Cc: stable@vger.kernel.org
> Signed-off-by: Pavitra Jha <jhapavitra98@gmail.com>

This patch no longer applies to the current drm-next code.  I've fixed
it up and attached it here.  It was a relatively large change so I
want to make sure you are ok with it if I keep your authorship.

Thanks!

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index b4ee5fc8e..9b55c56cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -564,6 +564,12 @@ static int amdgpu_discovery_get_table_info(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;
> +       }
> +
>         return 0;
>  }
>
> @@ -766,6 +772,14 @@ static void amdgpu_discovery_read_harvest_bit_per_ip(struct amdgpu_device *adev,
>         int i, j;
>
>         bhdr = (struct binary_header *)discovery_bin;
> +
> +       if (le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset) >=
> +           adev->discovery.size) {
> +               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));
> --
> 2.53.0
>

[-- Attachment #2: 0001-drm-amdgpu-discovery-validate-table-offset-before-IP.patch --]
[-- Type: text/x-patch, Size: 4804 bytes --]

From 4bebdc2f6911d5e114b6da58276884ded594ca61 Mon Sep 17 00:00:00 2001
From: Pavitra Jha <jhapavitra98@gmail.com>
Date: Wed, 8 Jul 2026 02:18:34 -0400
Subject: [PATCH] drm/amdgpu/discovery: validate table offset before IP
 discovery header cast

Sashiko AI review of the previous fix flagged three remaining gaps in
the discovery blob parser, all stemming from the same root cause: the
ip_discovery_header pointer itself is constructed from a firmware-
controlled offset with no validation before the cast.

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

This offset is a firmware-controlled u16 read directly from the
discovery blob's table_list, with no bounds check against
adev->discovery.size before being used to construct ihdr. Every
subsequent read from ihdr, including num_dies and die_info[], is
downstream of this unchecked pointer.

The other two items in that review (unbounded ip_offset advancement
via num_base_address, and num_dies exceeding die_info[]'s capacity)
were already addressed in the previous fix.

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
table_list[IP_DISCOVERY].offset directly rather than going through
get_table_info()). Add the equivalent check at that direct access site
as well, so all paths that construct an ip_discovery_header pointer
from a table offset are covered.

The check validates the offset itself against adev->discovery.size,
independent of any specific downstream struct size, since
get_table_info() is shared across ten different table types
(IP_DISCOVERY, HARVEST_INFO, GC, MALL_INFO, VCN_INFO, NPS_INFO, and
others) each with differently-sized table structures.

v2: rebase on latest code (Alex)

Fixes: d0c647a6aae2 ("drm/amdgpu/discovery: support new discovery binary header")
Signed-off-by: Pavitra Jha <jhapavitra98@gmail.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 31 ++++++++++++++++---
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index f33278fcc0f41..d565dd7abc6fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -566,22 +566,33 @@ static const char *amdgpu_discovery_get_fw_name(struct amdgpu_device *adev)
 }
 
 static struct table_info *
-amdgpu_discovery_get_table_info_from_bin(uint8_t *discovery_bin,
+amdgpu_discovery_get_table_info_from_bin(struct amdgpu_device *adev,
+					 uint8_t *discovery_bin,
 					 uint16_t table_id)
 {
 	struct binary_header *bhdr = (struct binary_header *)discovery_bin;
 	struct binary_header_v2 *bhdrv2;
+	struct table_info *info;
 
 	switch (bhdr->version_major) {
 	case 2:
 		bhdrv2 = (struct binary_header_v2 *)discovery_bin;
-		return &bhdrv2->table_list[table_id];
+		info = &bhdrv2->table_list[table_id];
+		break;
 	case 1:
 	case 0:
-		return &bhdr->table_list[table_id];
+		info = &bhdr->table_list[table_id];
+		break;
 	default:
 		return NULL;
 	}
+
+	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 NULL;
+	}
+	return info;
 }
 
 static int amdgpu_discovery_get_table_info(struct amdgpu_device *adev,
@@ -591,7 +602,8 @@ static int amdgpu_discovery_get_table_info(struct amdgpu_device *adev,
 	struct binary_header *bhdr =
 		(struct binary_header *)adev->discovery.bin;
 
-	*info = amdgpu_discovery_get_table_info_from_bin(adev->discovery.bin,
+	*info = amdgpu_discovery_get_table_info_from_bin(adev,
+							 adev->discovery.bin,
 							 table_id);
 	if (!*info) {
 		dev_err(adev->dev, "Invalid ip discovery table version %d\n",
@@ -803,6 +815,14 @@ static void amdgpu_discovery_read_harvest_bit_per_ip(struct amdgpu_device *adev,
 	int i, j;
 
 	bhdr = (struct binary_header *)discovery_bin;
+
+	if (le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset) >=
+	    adev->discovery.size) {
+		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));
@@ -1329,7 +1349,8 @@ static int amdgpu_discovery_sysfs_recurse(struct amdgpu_device *adev,
 	size_t ip_offset;
 	int ii, res;
 
-	info = amdgpu_discovery_get_table_info_from_bin(discovery_bin,
+	info = amdgpu_discovery_get_table_info_from_bin(adev,
+							discovery_bin,
 							IP_DISCOVERY);
 	if (!info)
 		return -EINVAL;
-- 
2.55.0

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

[Pavitra Jha]

Hi Alex,

Yes, absolutely! You can keep my authorship on the patch.

Thank you for fixing it up.

Best,
Pavitra Jha

On Wed, 8 Jul 2026 at 20:24, Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Wed, Jul 8, 2026 at 9:19 AM Pavitra Jha <jhapavitra98@gmail.com> wrote:
> >
> > Sashiko AI review of the previous fix flagged three remaining gaps in
> > the discovery blob parser, all stemming from the same root cause: the
> > ip_discovery_header pointer itself is constructed from a firmware-
> > controlled offset with no validation before the cast.
> >
> >   ihdr = (struct ip_discovery_header *)(discovery_bin +
> >                     le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset));
> >
> > This offset is a firmware-controlled u16 read directly from the
> > discovery blob's table_list, with no bounds check against
> > adev->discovery.size before being used to construct ihdr. Every
> > subsequent read from ihdr, including num_dies and die_info[], is
> > downstream of this unchecked pointer.
> >
> > The other two items in that review (unbounded ip_offset advancement
> > via num_base_address, and num_dies exceeding die_info[]'s capacity)
> > were already addressed in the previous fix.
> >
> > 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
> > table_list[IP_DISCOVERY].offset directly rather than going through
> > get_table_info()). Add the equivalent check at that direct access site
> > as well, so all paths that construct an ip_discovery_header pointer
> > from a table offset are covered.
> >
> > The check validates the offset itself against adev->discovery.size,
> > independent of any specific downstream struct size, since
> > get_table_info() is shared across ten different table types
> > (IP_DISCOVERY, HARVEST_INFO, GC, MALL_INFO, VCN_INFO, NPS_INFO, and
> > others) each with differently-sized table structures.
> >
> > Fixes: d0c647a6aae2 ("drm/amdgpu/discovery: support new discovery binary header")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Pavitra Jha <jhapavitra98@gmail.com>
>
> This patch no longer applies to the current drm-next code.  I've fixed
> it up and attached it here.  It was a relatively large change so I
> want to make sure you are ok with it if I keep your authorship.
>
> Thanks!
>
> Alex
>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > index b4ee5fc8e..9b55c56cb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> > @@ -564,6 +564,12 @@ static int amdgpu_discovery_get_table_info(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;
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -766,6 +772,14 @@ static void amdgpu_discovery_read_harvest_bit_per_ip(struct amdgpu_device *adev,
> >         int i, j;
> >
> >         bhdr = (struct binary_header *)discovery_bin;
> > +
> > +       if (le16_to_cpu(bhdr->table_list[IP_DISCOVERY].offset) >=
> > +           adev->discovery.size) {
> > +               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));
> > --
> > 2.53.0
> >