Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230712083137.1080883-1-ricardo.canuelo@collabora.com>
In-Reply-To: <20230712083137.1080883-1-ricardo.canuelo@collabora.com>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Wed, 12 Jul 2023 10:12:08 -0400
Message-ID: <CADnq5_OUS=JDfCdrCsuzTB0xD5yeX7piEDEqkRO-ffPTFVYs3g@mail.gmail.com>
Subject: Re: [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
To:     =?UTF-8?Q?Ricardo_Ca=C3=B1uelo?= <ricardo.canuelo@collabora.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     alexander.deucher@amd.com, kernel@collabora.com,
        linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jul 12, 2023 at 8:04=E2=80=AFAM Ricardo Ca=C3=B1uelo
<ricardo.canuelo@collabora.com> wrote:
>
> UBSAN complains about out-of-bounds array indexes on all 1-element
> arrays defined on this driver:
>
> UBSAN: array-index-out-of-bounds in /home/rcn/work/repos/kernelci/kernelc=
i-core/linux_kernel_mainline/drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwm=
gr/processpptables.c:1249:61
>
> Substitute them with proper flexible arrays.

+ Gustavo, Paulo

I haven't kept up with the flexible arrays stuff.  Is this equivalent
to a zero sized array?  We've been bitten by these kind of changes in
the past.  These structures define the layout of data in a rom image
on the board.  If the struct size changes, that could lead to errors
in the code that deals with these structures.

Alex

>
> Tested on an Acer R721T (grunt) Chromebook.
>
> Signed-off-by: Ricardo Ca=C3=B1uelo <ricardo.canuelo@collabora.com>
> ---
>  drivers/gpu/drm/amd/include/pptable.h | 36 +++++++++++++++------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/include/pptable.h b/drivers/gpu/drm/amd/=
include/pptable.h
> index 0b6a057e0a4c..a65e2807dc06 100644
> --- a/drivers/gpu/drm/amd/include/pptable.h
> +++ b/drivers/gpu/drm/amd/include/pptable.h
> @@ -473,14 +473,14 @@ typedef struct _ATOM_PPLIB_STATE_V2
>        /**
>        * Driver will read the first ucNumDPMLevels in this array
>        */
> -      UCHAR clockInfoIndex[1];
> +      __DECLARE_FLEX_ARRAY(UCHAR, clockInfoIndex);
>  } ATOM_PPLIB_STATE_V2;
>
>  typedef struct _StateArray{
>      //how many states we have
>      UCHAR ucNumEntries;
>
> -    ATOM_PPLIB_STATE_V2 states[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_STATE_V2, states);
>  }StateArray;
>
>
> @@ -491,7 +491,7 @@ typedef struct _ClockInfoArray{
>      //sizeof(ATOM_PPLIB_CLOCK_INFO)
>      UCHAR ucEntrySize;
>
> -    UCHAR clockInfo[1];
> +    __DECLARE_FLEX_ARRAY(UCHAR, clockInfo);
>  }ClockInfoArray;
>
>  typedef struct _NonClockInfoArray{
> @@ -501,7 +501,7 @@ typedef struct _NonClockInfoArray{
>      //sizeof(ATOM_PPLIB_NONCLOCK_INFO)
>      UCHAR ucEntrySize;
>
> -    ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_NONCLOCK_INFO, nonClockInfo);
>  }NonClockInfoArray;
>
>  typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
> @@ -514,7 +514,8 @@ typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_R=
ecord
>  typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Table
>  {
>      UCHAR ucNumEntries;                                                /=
/ Number of entries.
> -    ATOM_PPLIB_Clock_Voltage_Dependency_Record entries[1];             /=
/ Dynamically allocate entries.
> +    /* Dynamically allocate entries. */
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_Clock_Voltage_Dependency_Record, ent=
ries);
>  }ATOM_PPLIB_Clock_Voltage_Dependency_Table;
>
>  typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
> @@ -530,7 +531,8 @@ typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Record
>  typedef struct _ATOM_PPLIB_Clock_Voltage_Limit_Table
>  {
>      UCHAR ucNumEntries;                                                /=
/ Number of entries.
> -    ATOM_PPLIB_Clock_Voltage_Limit_Record entries[1];                  /=
/ Dynamically allocate entries.
> +    /* Dynamically allocate entries. */
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_Clock_Voltage_Limit_Record, entries)=
;
>  }ATOM_PPLIB_Clock_Voltage_Limit_Table;
>
>  union _ATOM_PPLIB_CAC_Leakage_Record
> @@ -554,7 +556,8 @@ typedef union _ATOM_PPLIB_CAC_Leakage_Record ATOM_PPL=
IB_CAC_Leakage_Record;
>  typedef struct _ATOM_PPLIB_CAC_Leakage_Table
>  {
>      UCHAR ucNumEntries;                                                 =
// Number of entries.
> -    ATOM_PPLIB_CAC_Leakage_Record entries[1];                           =
// Dynamically allocate entries.
> +    /* Dynamically allocate entries. */
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_CAC_Leakage_Record, entries);
>  }ATOM_PPLIB_CAC_Leakage_Table;
>
>  typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
> @@ -569,7 +572,8 @@ typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Record
>  typedef struct _ATOM_PPLIB_PhaseSheddingLimits_Table
>  {
>      UCHAR ucNumEntries;                                                 =
// Number of entries.
> -    ATOM_PPLIB_PhaseSheddingLimits_Record entries[1];                   =
// Dynamically allocate entries.
> +    /* Dynamically allocate entries. */
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_PhaseSheddingLimits_Record, entries)=
;
>  }ATOM_PPLIB_PhaseSheddingLimits_Table;
>
>  typedef struct _VCEClockInfo{
> @@ -581,7 +585,7 @@ typedef struct _VCEClockInfo{
>
>  typedef struct _VCEClockInfoArray{
>      UCHAR ucNumEntries;
> -    VCEClockInfo entries[1];
> +    __DECLARE_FLEX_ARRAY(VCEClockInfo, entries);
>  }VCEClockInfoArray;
>
>  typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
> @@ -593,7 +597,7 @@ typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Re=
cord
>  typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table
>  {
>      UCHAR numEntries;
> -    ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record entries[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record, entr=
ies);
>  }ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table;
>
>  typedef struct _ATOM_PPLIB_VCE_State_Record
> @@ -605,7 +609,7 @@ typedef struct _ATOM_PPLIB_VCE_State_Record
>  typedef struct _ATOM_PPLIB_VCE_State_Table
>  {
>      UCHAR numEntries;
> -    ATOM_PPLIB_VCE_State_Record entries[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VCE_State_Record, entries);
>  }ATOM_PPLIB_VCE_State_Table;
>
>
> @@ -627,7 +631,7 @@ typedef struct _UVDClockInfo{
>
>  typedef struct _UVDClockInfoArray{
>      UCHAR ucNumEntries;
> -    UVDClockInfo entries[1];
> +    __DECLARE_FLEX_ARRAY(UVDClockInfo, entries);
>  }UVDClockInfoArray;
>
>  typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record
> @@ -639,7 +643,7 @@ typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Re=
cord
>  typedef struct _ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table
>  {
>      UCHAR numEntries;
> -    ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record entries[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_UVD_Clock_Voltage_Limit_Record, entr=
ies);
>  }ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table;
>
>  typedef struct _ATOM_PPLIB_UVD_Table
> @@ -658,7 +662,7 @@ typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Recor=
d
>
>  typedef struct _ATOM_PPLIB_SAMClk_Voltage_Limit_Table{
>      UCHAR numEntries;
> -    ATOM_PPLIB_SAMClk_Voltage_Limit_Record entries[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_SAMClk_Voltage_Limit_Record, entries=
);
>  }ATOM_PPLIB_SAMClk_Voltage_Limit_Table;
>
>  typedef struct _ATOM_PPLIB_SAMU_Table
> @@ -676,7 +680,7 @@ typedef struct _ATOM_PPLIB_ACPClk_Voltage_Limit_Recor=
d
>
>  typedef struct _ATOM_PPLIB_ACPClk_Voltage_Limit_Table{
>      UCHAR numEntries;
> -    ATOM_PPLIB_ACPClk_Voltage_Limit_Record entries[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_ACPClk_Voltage_Limit_Record, entries=
);
>  }ATOM_PPLIB_ACPClk_Voltage_Limit_Table;
>
>  typedef struct _ATOM_PPLIB_ACP_Table
> @@ -745,7 +749,7 @@ typedef struct ATOM_PPLIB_VQ_Budgeting_Record{
>  typedef struct ATOM_PPLIB_VQ_Budgeting_Table {
>      UCHAR revid;
>      UCHAR numEntries;
> -    ATOM_PPLIB_VQ_Budgeting_Record         entries[1];
> +    __DECLARE_FLEX_ARRAY(ATOM_PPLIB_VQ_Budgeting_Record, entries);
>  } ATOM_PPLIB_VQ_Budgeting_Table;
>
>  #pragma pack()
> --
> 2.25.1
>