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

[Tasos Sahanidis <tasos@tasossah.com>]

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

> >  }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)))
      |                                                                       ^~~~~~