On 7/9/26 8:19 AM, Kamal, Asad wrote: > AMD General > > Hi @John Olender, > > amdgpu_atom_parse_data_header() does not validate usStructureSize against the > BIOS boundary — it reads the value directly from the BIOS bytes without any > bounds check: > > if (size) > *size = CU16(idx); /* raw BIOS read, no validation */ > So soft_pp_table_size is whatever the VBIOS claims and cannot be trusted for > the VBIOS path. >
Just before this assignment is where I'm suggesting the parsed table ends be sanity checked against the vbios size. A table whose end blatantly overflows the vbios would then be rejected early. This would avoid redundant checks in the inline pp_entries_max(). Thanks, John > Regarding other ATOM tables — the PP table is unique in that the host driver > parses its sub-tables entry-by-entry, using ucNumEntries from the BIOS to > drive kzalloc() sizes and loop bounds. That is the direct exploit path this > series addresses. Most other ATOM tables are either passed raw to firmware, > read as a fixed-size struct, or executed as ATOM scripts — none use a > BIOS-supplied count to allocate kernel memory in a loop, so an inflated > usStructureSize is harmless for them. > > The bios_end check in pp_entries_max() is still needed for the VBIOS path and > the !hardcode_pp_table condition correctly gates it. > > Thanks & Regards > Asad > > -----Original Message----- > From: John Olender <[email protected]> > Sent: Thursday, July 9, 2026 4:09 PM > To: Kamal, Asad <[email protected]>; Alex Deucher <[email protected]> > Cc: [email protected]; Feng, Kenneth <[email protected]>; > Deucher, Alexander <[email protected]>; Lazar, Lijo > <[email protected]>; Zhang, Hawking <[email protected]>; Wang, > Yang(Kevin) <[email protected]> > Subject: Re: [PATCH] drm/amd/pm: Remove vbios bounds check from > pp_entries_max() > > On 7/9/26 5:57 AM, Kamal, Asad wrote: >> AMD General >> >> Hi @John Olender, >> >> Good catch. However, as Alex points out, removing the check entirely drops >> protection for VBIOS-sourced tables. >> >> Instead of removing the check, could you condition it on hardcode_pp_table? >> When it is NULL we are on the VBIOS path and the bios_end check should stay. >> When it is non-NULL a custom table was uploaded via sysfs and the check can >> be skipped — soft_pp_table_size is already set to the kernel-supplied upload >> size. >> >> if (!hwmgr->hardcode_pp_table) { >> const char *bios_end = (const char *)adev->bios + adev->bios_size; >> if (pp_end > bios_end) >> return 0; >> } >> >> This fixes your regression while keeping Alex's concern addressed. Please >> let me know if it ok with you. >> >> Thanks & Regards >> Asad > > Hmm. > > Checking for vbios overflow when a table header is parsed should allow > pp_entries_max() to implicitly detect vbios overflow for the vbios-provided > pp_table. > > Is there a reason all atom tables are not sanity checked in this way? > It seems odd that the pp_table header isn't trusted while the rest of the > table headers are. > > Thanks, > John > >> -----Original Message----- >> From: amd-gfx <[email protected]> On Behalf Of >> Alex Deucher >> Sent: Wednesday, July 8, 2026 10:08 PM >> To: John Olender <[email protected]> >> Cc: [email protected]; Feng, Kenneth >> <[email protected]>; Deucher, Alexander >> <[email protected]>; Kamal, Asad <[email protected]>; Lazar, >> Lijo <[email protected]>; Zhang, Hawking <[email protected]>; >> Wang, Yang(Kevin) <[email protected]> >> Subject: Re: [PATCH] drm/amd/pm: Remove vbios bounds check from >> pp_entries_max() >> >> On Sat, Jul 4, 2026 at 6:19 AM John Olender <[email protected]> wrote: >>> >>> The soft_pp_table is not contained within the vbios when using a >>> pp_table override since commit 3cfe433630508 ("drm/amd/pm: Use >>> uploaded size for legacy custom PPTable"). >>> >>> Fixes: e30b3e3ab51ad ("drm/amdgpu/pm: add pp_entries_max() helper") >>> Signed-off-by: John Olender <[email protected]> >> >> Removing this will effectively remove the bounds check from tables read from >> the bios. I don't think we want to do that. >> >> Alex >> >>> --- >>> drivers/gpu/drm/amd/pm/powerplay/inc/hwmgr.h | 4 ---- >>> 1 file changed, 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/pm/powerplay/inc/hwmgr.h >>> b/drivers/gpu/drm/amd/pm/powerplay/inc/hwmgr.h >>> index 7ebc1344023ff..19023a850e882 100644 >>> --- a/drivers/gpu/drm/amd/pm/powerplay/inc/hwmgr.h >>> +++ b/drivers/gpu/drm/amd/pm/powerplay/inc/hwmgr.h >>> @@ -833,14 +833,10 @@ static inline uint32_t pp_entries_max(const struct >>> pp_hwmgr *hwmgr, >>> const void *sub_table, >>> size_t hdr_size, size_t >>> rec_size) { >>> - struct amdgpu_device *adev = (struct amdgpu_device *)hwmgr->adev; >>> - const char *bios_end = (const char *)adev->bios + adev->bios_size; >>> const char *pp_end = (const char *)hwmgr->soft_pp_table >>> + hwmgr->soft_pp_table_size; >>> const char *entries = (const char *)sub_table + hdr_size; >>> >>> - if (pp_end > bios_end) >>> - return 0; >>> if (!rec_size || entries >= pp_end) >>> return 0; >>> return (uint32_t)((pp_end - entries) / rec_size); >>> >>> base-commit: e3cc8c108f9a91728f9ff6e24cbf3b41c58b8f8b >>> -- >>> 2.47.3 >>> >
