On Wed, Apr 10, 2024 at 3:37 AM Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxx> (v1)
> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > ---
> > 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
