* Re: [PATCH v2] drm/amdgpu/discovery: validate table offset before IP discovery header cast
2026-07-08 6:18 ` [PATCH v2] drm/amdgpu/discovery: validate table offset before IP discovery header cast Pavitra Jha
@ 2026-07-08 14:54 ` Alex Deucher
2026-07-08 15:35 ` Pavitra Jha
0 siblings, 1 reply; 3+ messages in thread
From: Alex Deucher @ 2026-07-08 14:54 UTC (permalink / raw)
To: Pavitra Jha
Cc: alexander.deucher, christian.koenig, airlied, simona, amd-gfx,
dri-devel, linux-kernel
[-- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread