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

[Alex Deucher]

On Mon, May 27, 2024 at 5:42 AM Tasos Sahanidis <tasos at tasossah.com> wrote:
>
> On 2024-05-23 17:52, Alex Deucher wrote:
> > On Thu, May 23, 2024 at 9:05 AM Tasos Sahanidis <tasos at tasossah.com> 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 at tasossah.com>
> >> ---
> >>  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?

Yes, that's fine.  Thanks!

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

yes.

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

Yes.

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

I'm not following.  Why not?

Alex

>
> Please let me know what you think.
>
> Thanks!
>
> --
> Tasos