[PATCH] drm/amd/pm: Remove vbios bounds check from pp_entries_max()

[PATCH] drm/amd/pm: Remove vbios bounds check from pp_entries_max()

[John Olender]

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 <john.olender@gmail.com>
---
 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

Re: [PATCH] drm/amd/pm: Remove vbios bounds check from pp_entries_max()

[Alex Deucher]

On Sat, Jul 4, 2026 at 6:19 AM John Olender <john.olender@gmail.com> 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 <john.olender@gmail.com>

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
>

RE: [PATCH] drm/amd/pm: Remove vbios bounds check from pp_entries_max()

[Kamal, Asad]

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
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex Deucher
Sent: Wednesday, July 8, 2026 10:08 PM
To: John Olender <john.olender@gmail.com>
Cc: amd-gfx@lists.freedesktop.org; Feng, Kenneth <Kenneth.Feng@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Kamal, Asad <Asad.Kamal@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
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 <john.olender@gmail.com> 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 <john.olender@gmail.com>

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
>

Re: [PATCH] drm/amd/pm: Remove vbios bounds check from pp_entries_max()

[John Olender]

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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex Deucher
> Sent: Wednesday, July 8, 2026 10:08 PM
> To: John Olender <john.olender@gmail.com>
> Cc: amd-gfx@lists.freedesktop.org; Feng, Kenneth <Kenneth.Feng@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Kamal, Asad <Asad.Kamal@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 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 <john.olender@gmail.com> 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 <john.olender@gmail.com>
> 
> 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
>>

RE: [PATCH] drm/amd/pm: Remove vbios bounds check from pp_entries_max()

[Kamal, Asad]

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 <john.olender@gmail.com>
Sent: Thursday, July 9, 2026 4:09 PM
To: Kamal, Asad <Asad.Kamal@amd.com>; Alex Deucher <alexdeucher@gmail.com>
Cc: amd-gfx@lists.freedesktop.org; Feng, Kenneth <Kenneth.Feng@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Alex Deucher
> Sent: Wednesday, July 8, 2026 10:08 PM
> To: John Olender <john.olender@gmail.com>
> Cc: amd-gfx@lists.freedesktop.org; Feng, Kenneth
> <Kenneth.Feng@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Kamal, Asad <Asad.Kamal@amd.com>; Lazar,
> Lijo <Lijo.Lazar@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
> Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 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 <john.olender@gmail.com> 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 <john.olender@gmail.com>
>
> 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
>>

Re: [PATCH] drm/amd/pm: Remove vbios bounds check from pp_entries_max()

[John Olender]

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 <john.olender@gmail.com>
> Sent: Thursday, July 9, 2026 4:09 PM
> To: Kamal, Asad <Asad.Kamal@amd.com>; Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx@lists.freedesktop.org; Feng, Kenneth <Kenneth.Feng@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Alex Deucher
>> Sent: Wednesday, July 8, 2026 10:08 PM
>> To: John Olender <john.olender@gmail.com>
>> Cc: amd-gfx@lists.freedesktop.org; Feng, Kenneth
>> <Kenneth.Feng@amd.com>; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; Kamal, Asad <Asad.Kamal@amd.com>; Lazar,
>> Lijo <Lijo.Lazar@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
>> Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
>> 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 <john.olender@gmail.com> 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 <john.olender@gmail.com>
>>
>> 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
>>>
>