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 <[email protected]> wrote: > > On Wed, Jul 8, 2026 at 9:19 AM Pavitra Jha <[email protected]> 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: [email protected] > > Signed-off-by: Pavitra Jha <[email protected]> > > 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 > >
