Re: [PATCH] drm/amdgpu/pptable: Fix UBSAN array-index-out-of-bounds

[Tasos Sahanidis <tasos@xxxxxxxxxxxx>]

On 2024-05-23 17:52, Alex Deucher wrote:
> On Thu, May 23, 2024 at 9:05 AM Tasos Sahanidis <tasos@xxxxxxxxxxxx> wrote:
>>
>> Dyanmically sized arrays used [1] instead of []. Replacing the former
>> with the latter resolves multiple warnings observed on boot with a
>> BONAIRE card.
>>
>> Signed-off-by: Tasos Sahanidis <tasos@xxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/amd/include/pptable.h | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
>> index 2e8e6c9875f6..d1dec880d2d6 100644
>> --- a/drivers/gpu/drm/amd/include/pptable.h
>> +++ b/drivers/gpu/drm/amd/include/pptable.h
>> @@ -480,7 +480,7 @@ typedef struct _StateArray{
>>      //how many states we have
>>      UCHAR ucNumEntries;
>>
>> -    ATOM_PPLIB_STATE_V2 states[1];
>> +    ATOM_PPLIB_STATE_V2 states[];
> 
> Can you add __counted_by(ucNumEntries) to the end of the line? E.g.,
> 
> ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
> 
> Same comment for the other changes below.
> 
> Alex

Sure thing! I have the v2 ready and will submit after testing it on
hardware. I do have a question though. The following already uses [].
Would it be acceptable to also modify it in the same patch?

@@ -658,7 +658,7 @@ typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Record
 
 typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Table{
     UCHAR numEntries;
-    ATOM_PPLIB_SAMClk_Voltage_Limit_Record entries[];
+    ATOM_PPLIB_SAMClk_Voltage_Limit_Record entries[] __counted_by(numEntries);
 }ATOM_PPLIB_SAMClk_Voltage_Limit_Table;
 
 typedef struct _ATOM_PPLIB_SAMU_Table

There's also this, which I think __counted_by can be used here as well:

diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/include/pptable.h
index febc853d2a07..341d4a4e8d98 100644
--- a/drivers/gpu/drm/amd/include/pptable.h
+++ b/drivers/gpu/drm/amd/include/pptable.h
@@ -497,15 +497,15 @@ typedef struct _ClockInfoArray{
 typedef struct _NonClockInfoArray{
 
     //how many non-clock levels we have. normally should be same as number of states
     UCHAR ucNumEntries;
     //sizeof(ATOM_PPLIB_NONCLOCK_INFO)
     UCHAR ucEntrySize;
     
-    ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[];
+    ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[] __counted_by(ucNumEntries);
 }NonClockInfoArray;
 
 typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
 {
     USHORT usClockLow;
     UCHAR  ucClockHigh;
     USHORT usVoltage;

All the other ones are UCHAR, so __counted_by can not be used on them.

Please let me know what you think.

Thanks!

--
Tasos