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

[Alex Deucher]

On Thu, May 30, 2024 at 6:31 AM Tasos Sahanidis <tasos at tasossah.com> wrote:
>
> On Mon, May 27, 2024 at 10:47:39AM -0400, Alex Deucher wrote:
> > 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
> > >
>
> Apologies for the delay. I realised my compiler actually didn't support
> the attribute, so I had to install clang 19 from git.
>
> After doing so, I saw this warning about ATOM_PPLIB_STATE_V2.
>
> drivers/gpu/drm/amd/amdgpu/../include/pptable.h:483:5: warning: 'counted_by' should not be applied to an array with element of unknown size because 'ATOM_PPLIB_STATE_V2'
>  (aka 'struct _ATOM_PPLIB_STATE_V2') is a struct type with a flexible array member. This will be an error in a future compiler version [-Wbounds-safety-counted-by-elt-type-unknown-size]
>   483 |     ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries);
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Thus I'll remove the __counted_by() from that one.

just leave it there commented out.  E.g.,

ATOM_PPLIB_STATE_V2 states[] /* __counted_by(ucNumEntries) */;

for reference so if the compiler ever gets updated to handle this, it
can be re-enabled.

>
> > >  }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
>
> If I'm not mistaken, for a UCHAR flexible array such as clockInfo[],
> we would then need to multiply the count by the size, which results in:
>
> drivers/gpu/drm/amd/amdgpu/../include/pptable.h:494:36: error: 'counted_by' argument must be a simple declaration reference
>   494 |     UCHAR clockInfo[] __counted_by(ucEntrySize * ucNumEntries);
>       |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
> ./include/linux/compiler_attributes.h:105:62: note: expanded from macro '__counted_by'
>   105 | # define __counted_by(member)           __attribute__((__counted_by__(member)))
>       |                                                                       ^~~~~~

Ah, yes, we can skip those.

Alex