Date:   Fri, 11 Nov 2022 19:05:23 +1300
From:   Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     Alex Deucher <alexdeucher@...il.com>,
        linux-hardening@...r.kernel.org,
        Christian König <christian.koenig@....com>,
        amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [RFC] Approaches to deal with a struct with multiple fake
 flexible arrays members

On Thu, Nov 10, 2022 at 11:39:02PM -0600, Gustavo A. R. Silva wrote:
> On Wed, Nov 09, 2022 at 10:20:34PM -0500, Alex Deucher wrote:
> > On Wed, Nov 9, 2022 at 8:31 PM Paulo Miguel Almeida
> > <paulo.miguel.almeida.rodenas@...il.com> wrote:
> > >
> > > On Wed, Nov 09, 2022 at 06:45:57PM -0600, Gustavo A. R. Silva wrote:
> > > > On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
> > > >
> > > > Adding Alex, Christian and DRM lists to the thread.
> > >
> > > Thanks so much for your reply Gustavo
> > > Yep, that's a good idea.
> > >
> > > >
> > > > > struct _ATOM_INIT_REG_BLOCK {
> > > > >     USHORT                     usRegIndexTblSize;    /*     0     2 */
> > > > >     USHORT                     usRegDataBlkSize;     /*     2     2 */
> > > > >     ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
> > > > >     ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */
> > > >
> > > > I didn't find evidence that asRegDataBuf is used anywhere in the
> > > > codebase[1].
> > > > ...
> > > > <snip>
> > > > ...
> > > > As I pointed out above, I don't see asRegDataBuf[] being used in the
> > > > codebase (of course it may describe firmware memory layout). Instead,
> > > > there is this jump to a block of data past asRegIndexBuf[]:
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
> > > > 1444: ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
> > > > 1445:         (ATOM_MEMORY_SETTING_DATA_BLOCK *)
> > > > 1446:         ((u8 *)reg_block + (2 * sizeof(u16)) +
> > > > 1447:                  le16_to_cpu(reg_block->usRegIndexTblSize));
> > > >
> > > > So, it seems the one relevant array, from the kernel side, is
> > > > asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
> > > > structure... or if we could try modifying that struct and only have
> > > > asRegIndexBuf[] as last member? and then we can transform it into a
> > > > flex-array member.
> > >
> > > I saw that one too. That would be the way it's currently accessing
> > > asRegDataBuf member as the existing struct would make asRegDataBuf[0] point
> > > to some index within the asRegIndexBuf member (as you probably got it too)
> > >
> > > you are right... asRegDataBuff isn't used from a static analysis
> > > point of view but removing it make the code a bit cryptic, right?
> > >
> > > That's pickle, ay? :-)
> > >
> > > >
> > > > If for any strong reasong we cannot remove asRegDataBuf[] then I think we
> > > > could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
> > > > in the structure.
> > > >
> > >
> > > Out of curiosity, why both rather than just asRegIndexBuf?
> 
> Because if I understand the code and Alex's reply below correctly, both
> arrays are being used to describe data of variable size, and both arrays
> need to stay. The situation is that it'd be _strange_ to transform just
> one of them into a flex-array member and not the other, and at the same

My apologies, I tried being succinct and I ended up mistakenly leading you 
to understand a different thing. I will be more careful next time :-)

What I meant was why would you use DECLARE_FLEX_ARRAY macro for both
members instead of the following ?

typedef struct _ATOM_INIT_REG_BLOCK{
   USHORT                           usRegIndexTblSize;
   USHORT                           usRegDataBlkSize;
   DECLARE_FLEX_ARRAY(ATOM_INIT_REG_INDEX_FORMAT, asRegIndexBuf);  // Macro needed
   ATOM_MEMORY_SETTING_DATA_BLOCK   asRegDataBuf[];		   // Regular FMA
}ATOM_INIT_REG_BLOCK;

> On the other hand, I fail to see how the current state of the code is
> problematic for things like -fstrict-flex-arrays, right now. asRegDataBuf[]
> is not being used for anything in the kernel, and asRegIndexBuf[] is
> correctly being accessed through it's only valid index zero, after which
> is decays into a pointer[2].
> 
> That struct is definitely not great (I don't love it), but we might try
> to explore other cases while we determine how and if we ultimately need
> to modify it.
> 
> I'll open an issue for this in the bug tracker, so we keep an eye on it.
> :)

Fair enough. Thanks heaps Gustavo, I will move on to the other fake flex
array occurences and circle back to it once a decision in made. Please
count on me to make the changes :-)

> 
> > >
> > > > But first, of course, Alex, Christian, it'd be really great if we can
> > > > have your input and feedback. :)
> > 
> > This header describes the layout of memory stored in the ROM on the
> > GPU.  It's shared between vbios and driver so some parts aren't
> > necessarily directly used by the driver.  As for what to do about it,
> > I'm not sure.  This structure stores a set of register offsets
> > (asRegIndexBuf) and data values (asRegDataBuf) for specific operations
> > (e.g., walk this structure and program register X with value Y.  For a
> > little background on atombios, it's a set of data and command tables
> > stored in the vbios ROM image.  The driver has an interpreter for the
> > command tables (see atom.c) so it can parse the command tables to
> > initialize the asic to a usable state.  The various programming
> > sequences vary depending on the components the AIB/OEM uses for the
> > board (different vram vendors, different clock/voltage settings,
> > etc.).  The legacy VGA/VBE and the GOP driver and the OS driver can
> > use these tables, so this allows us to initialize any GPU in a generic
> > way on any architecture even if the platform firmware doesn't post the
> > card.  For the most part the driver doesn't have to deal with these
> > particular tables directly, other than for some very specific cases
> > where the driver needs to grab some elements from the tables to
> > populate the power management controller for GPU memory reclocking
> > parameters.  However, the command tables as interpreted by the parser
> > very often will directly parse these tables.  So you might have a
> > command table that the driver executes to initialize some part of the
> > GPU which ultimately fetches the table from the ROM image and walks it
> > programming registers based on the offset and values in that table.
> > So if you were debugging something that involves the atombios parser
> > and walking through one of the command tables, you may be confused if
> > the data tables don't match what header says.  So ideally, we'd keep
> > both arrays.
> 
> Thanks a lot for the input, Alex.
> --
> Gustavo
> 

Same here, thanks heaps Alex!

- Paulo A.

Powered by blists - more mailing lists