On 7/10/26 11:41 AM, Kamal, Asad wrote: > AMD General > > Hi @John Olender, > > You are right — ctx->bios_size is available in struct atom_context so the > check is straightforward to add in amdgpu_atom_parse_data_header(). If that > validation lands, soft_pp_table_size will be bounded at parse time, making > the bios_end check in pp_entries_max() redundant for the VBIOS path. > > That would be the cleaner two-step solution: > > 1) Add usStructureSize bounds check in amdgpu_atom_parse_data_header() — > benefits all ATOM data tables > 2) Remove the bios_end check from pp_entries_max() — now safe since > parse-time validation guarantees pp_end ≤ bios_end > > However, that is a change to the core ATOM parser touching all data table > callers — it needs its own patch and review. If you want to send that patch, > we can then simplify pp_entries_max() accordingly. > > In the meantime, I will send a follow-up patch that conditions the bios_end > check on !hardcode_pp_table to fix the custom PP table regression while > keeping Alex's VBIOS protection in place. > > Thanks > Asad
Sounds good. Thanks, John > > -----Original Message----- > From: John Olender <[email protected]> > Sent: Thursday, July 9, 2026 7: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 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 >>>> >> >
