[PATCH] drm/radeon: silence UBSAN warning (v2)

[Alex Deucher]

On Wed, Apr 10, 2024 at 3:37 AM Kees Cook <keescook at chromium.org> wrote:
>
> On Mon, Apr 08, 2024 at 01:37:48PM -0400, Alex Deucher wrote:
> > Convert a variable sized array from [1] to [].
> >
> > v2: fix up a few more.
> >
> > Acked-by: Christian König <christian.koenig at amd.com> (v1)
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > ---
> >  drivers/gpu/drm/radeon/pptable.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/pptable.h b/drivers/gpu/drm/radeon/pptable.h
> > index 94947229888ba..3493b1f979fed 100644
> > --- a/drivers/gpu/drm/radeon/pptable.h
> > +++ b/drivers/gpu/drm/radeon/pptable.h
> > @@ -432,7 +432,7 @@ typedef struct _ATOM_PPLIB_STATE_V2
> >        /**
> >        * Driver will read the first ucNumDPMLevels in this array
> >        */
> > -      UCHAR clockInfoIndex[1];
> > +      UCHAR clockInfoIndex[];
> >  } ATOM_PPLIB_STATE_V2;
>
> The comment slightly above this hunk says:
>
>       //number of valid dpm levels in this state; Driver uses it to calculate the whole
>       //size of the state: sizeof(ATOM_PPLIB_STATE_V2) + (ucNumDPMLevels - 1) * sizeof(UCHAR)
>
> This is wrong now, as ATOM_PPLIB_STATE_V2 isn't over-sized now. It
> should be:
>
>       //size of the state: sizeof(ATOM_PPLIB_STATE_V2) + (ucNumDPMLevels) * sizeof(UCHAR)
>
> or better yet, struct_size(ATOM_PPLIB_STATE_V2, clockInfoIndex, ucNumDPMLevels)
>

Will update that.

> I couldn't easily find any "sizeof" uses against these structs, but are
> you sure there aren't size changes associated with this adjustment?
>

I also didn't see any.

> Also, if the comment is accurate, then clockInfoIndex can also gain a
> __counted_by annotation:
>
>         UCHAR clockInfoIndex[] __counted_by(ucNumDPMLevels);
>
> The use of __counted_by() seems like it could apply to the other changes
> as well?

Yes, will fix that up too.

>
> >
> >  typedef struct _StateArray{
>
> This has a fake flex-array as well, but it's a flex array of flex
> arrays. :(
>
> typedef struct _StateArray{
>     //how many states we have
>     UCHAR ucNumEntries;
>
>     ATOM_PPLIB_STATE_V2 states[1];
> }StateArray;
>
> I suspect get_state_entry_v2() may trip the runtime checking too, though
> it's using a bunch of casted pointer math instead of direct array
> accesses, so maybe it's avoid the instrumentation for now?

Yes, I think so.  I don't think it was UBSAN, but IIRC, some compiler
versions complained about it at the time or something like that.  I
don't remember exactly what.  Too long ago.

Alex

>
> > @@ -450,7 +450,7 @@ typedef struct _ClockInfoArray{
> >      //sizeof(ATOM_PPLIB_CLOCK_INFO)
> >      UCHAR ucEntrySize;
> >
> > -    UCHAR clockInfo[1];
> > +    UCHAR clockInfo[];
> >  }ClockInfoArray;
> >
> >  typedef struct _NonClockInfoArray{
> > @@ -460,7 +460,7 @@ typedef struct _NonClockInfoArray{
> >      //sizeof(ATOM_PPLIB_NONCLOCK_INFO)
> >      UCHAR ucEntrySize;
> >
> > -    ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[1];
> > +    ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[];
> >  }NonClockInfoArray;
> >
> >  typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
>
> -Kees
>
> --
> Kees Cook