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;
Date:   Fri, 14 Jul 2023 22:40:12 +1200
From:   Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
To:     Alex Deucher <alexdeucher@gmail.com>
Cc:     Ricardo =?utf-8?Q?Ca=C3=B1uelo?= <ricardo.canuelo@collabora.com>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        alexander.deucher@amd.com, kernel@collabora.com,
        linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
Message-ID: <ZLEmDBfklhfGCLGa@mail.google.com>
References: <20230712083137.1080883-1-ricardo.canuelo@collabora.com>
 <CADnq5_OUS=JDfCdrCsuzTB0xD5yeX7piEDEqkRO-ffPTFVYs3g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CADnq5_OUS=JDfCdrCsuzTB0xD5yeX7piEDEqkRO-ffPTFVYs3g@mail.gmail.com>
Precedence: bulk

On Wed, Jul 12, 2023 at 10:12:08AM -0400, Alex Deucher wrote:
> On Wed, Jul 12, 2023 at 8:04 AM Ricardo Cañuelo
> <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/kernelci-core/linux_kernel_mainline/drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/processpptables.c:1249:61
> >
> > Substitute them with proper flexible arrays.
> 

Hi Ricardo,

thanks for your patch! when submitting patches regarding flex-arrays
please cc linux-hardening@vger.kernel.org too :-)

I added my considerations in-line.

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

Hi Alex,

correct, both zero-sized and one-sized arrays (in some contexts)
are deprecated [1] and being replaced with flexible arrays as part of the 
kernel self protection project (KSSP) led by Kees Cook and Gustavo Silva.

Converting away from them can be tricky, and I think such conversions need
to explicitly show how they were checked for binary differences[2].

Ricardo, can you please check for deltas and report your findings in the
commit log?

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
[2] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/

> 
> >
> > Tested on an Acer R721T (grunt) Chromebook.
> >
> > Signed-off-by: Ricardo Cañuelo <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);

_All_ references of __DECLARE_FLEX_ARRAY in this patch should be replaced
with DECLARE_FLEX_ARRAY macro (without __).

While they converge to the same code today, __DECLARE_FLEX_ARRAY macro
is specific for UAPI while DECLARE_FLEX_ARRAY isn't as seen in this
patch:

https://github.com/torvalds/linux/commit/3080ea5553cc909b000d1f1d964a9041962f2c5b

> >  } ATOM_PPLIB_STATE_V2;
> >
[... snip for brevity ]
> >
> >  typedef struct _VCEClockInfo{
> > @@ -581,7 +585,7 @@ typedef struct _VCEClockInfo{
> >
> >  typedef struct _VCEClockInfoArray{
> >      UCHAR ucNumEntries;
> > -    VCEClockInfo entries[1];
> > +    __DECLARE_FLEX_ARRAY(VCEClockInfo, entries);
> >  }VCEClockInfoArray;

this would cause a binary difference if the driver code used sizeof().

Currently, the source code is calculating the struct size manually at

drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c:86

  86 static uint16_t get_vce_clock_info_array_size(struct pp_hwmgr *hwmgr,
  87                         const ATOM_PPLIB_POWERPLAYTABLE *powerplay_table)
  88 {
  89         uint16_t table_offset = get_vce_clock_info_array_offset(hwmgr,
  90                                                         powerplay_table);
  91         uint16_t table_size = 0;
  92 
  93         if (table_offset > 0) {
  94                 const VCEClockInfoArray *p = (const VCEClockInfoArray *)
  95                         (((unsigned long) powerplay_table) + table_offset);
  96                 table_size = sizeof(uint8_t) + p->ucNumEntries * sizeof(VCEClockInfo);
  97         }
  98 
  99         return table_size;
 100 }

 Ricardo, please replace table_size assigment with struct_size(p, entries, p->ucNumEntries)

> >
> >  typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
> > @@ -593,7 +597,7 @@ typedef struct _ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record
> >  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, entries);
> >  }ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table;

similar situation as above.. needs struct_size

drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c:115

 115 static uint16_t get_vce_clock_voltage_limit_table_size(struct pp_hwmgr *hwmgr,
 116                                                         const ATOM_PPLIB_POWERPLAYTABLE *powerplay_table)
 117 {
 118         uint16_t table_offset = get_vce_clock_voltage_limit_table_offset(hwmgr, powerplay_table);
 119         uint16_t table_size = 0;
 120
 121         if (table_offset > 0) {
 122                 const ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *ptable =
 123                         (const ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *)(((unsigned long) powerplay_table) + table_offset);
 124
 125                 table_size = sizeof(uint8_t) + ptable->numEntries * sizeof(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record);
 126         }
 127         return table_size;
 128 }

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

On drivers/gpu/drm/amd/pm/legacy-dpm:420, there are many references that
are calculated manually and could possibly benefit from this
housekeeping patch about flex arrays.

In most cases, changing the struct like you did would create binary
differences but given the way that this driver was implemented, they can
slip through the cracks and make those lines more cryptic and
subsequently, less likely to be fixed in the future....

my 2 cents would be to tidy this up alongside the struct changes whether
that's in the same patch or a series of patches.

 420                         VCEClockInfoArray *array = (VCEClockInfoArray *)
 421                                 (mode_info->atom_context->bios + data_offset +
 422                                  le16_to_cpu(ext_hdr->usVCETableOffset) + 1);
 423                         ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *limits =
 424                                 (ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *)
 425                                 (mode_info->atom_context->bios + data_offset +
 426                                  le16_to_cpu(ext_hdr->usVCETableOffset) + 1 +
 427                                  1 + array->ucNumEntries * sizeof(VCEClockInfo));
 428                         ATOM_PPLIB_VCE_State_Table *states =
 429                                 (ATOM_PPLIB_VCE_State_Table *)
 430                                 (mode_info->atom_context->bios + data_offset +
 431                                  le16_to_cpu(ext_hdr->usVCETableOffset) + 1 +
 432                                  1 + (array->ucNumEntries * sizeof (VCEClockInfo)) +
 433                                  1 + (limits->numEntries * sizeof(ATOM_PPLIB_VCE_Clock_Voltage_Limit_Record)));


> >  #pragma pack()
> > --
> > 2.25.1
> >

I didn't review all struct changes but I reckon that you got the train
of thought to be followed by now. Please count on me for reviewing those
changes :-)

Thanks!

- Paulo A.