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.
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
>>
