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

[Kees Cook <keescook@xxxxxxxxxxxx>]

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)

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

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?

>  
>  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?

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