linux-hardening - Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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

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?

> But first, of course, Alex, Christian, it'd be really great if we can
> have your input and feedback. :)
> 
> Thanks!
> --
> Gustavo
> 

- Paulo A.