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