On Tue, Jun 11, 2024 at 11:26:25PM -0700, srinivas pandruvada wrote:
> On Sat, 2024-06-08 at 11:56 +0200, Erick Archer wrote:
> > Hi Srinivas,
> > First of all, thanks for looking at this ;)
> >
> > On Sat, Jun 08, 2024 at 01:42:54AM -0700, srinivas pandruvada wrote:
> > > On Sun, 2024-05-26 at 15:32 +0200, Erick Archer wrote:
> > > > One-element arrays as fake flex arrays are deprecated [1] and we
> > > > are
> > > > moving towards adopting C99 flexible-array members, instead. This
> > > > case
> > > > also has more complexity because it is a flexible array of
> > > > flexible
> > > > arrays and this patch needs to be ready to enable the new
> > > > compiler
> > > > flag
> > > > -Wflex-array-member-not-at-end (coming in GCC-14) globally.
> > > >
> > > > So, define a new struct type for the single reports:
> > > >
> > > > struct report {
> > > > uint16_t size;
> > > > struct hostif_msg_hdr msg;
> > > > } __packed;
> > > >
> > > > but without the payload (flex array) in it. And add this payload
> > > > to
> > > > the
> > > > "hostif_msg" structure. This way, in the "report_list" structure
> > > > we
> > > > can
> > > > declare a flex array of single reports which now do not contain
> > > > another
> > > > flex array.
> > > >
> > > > struct report_list {
> > > > [...]
> > > > struct report reports[];
> > > > } __packed;
> > > >
> > > > Also, use "container_of()" whenever we need to retrieve a pointer
> > > > to
> > > > the flexible structure, through which we can access the flexible
> > > > array
> > > > if needed.
> > > >
> > > > Moreover, refactor the code accordingly to use the new structures
> > > > and
> > > > take advantage of this avoiding some pointer arithmetic and using
> > > > the
> > > > "struct_size" helper when possible.
> > > >
> > > > This way, the code is more readable and safer.
> > >
> > > Applied and tested, atleast didn't break anything.
> > >
> > > But the explanation above didn't give me enough clue. You have
> > > added a
> > > payload[] in the struct hostif_msg {} then using that as a message
> > > pointer following the header. I think this description needs to be
> > > better.
> >
> > Yeah, I will try to improve the commit message. What do you think
> > about
> > the following parragrafs?
> >
> > [I have copied part of the message to show where the new info will
> > be]
> > > > declare a flex array of single reports which now do not contain
> > > > another flex array.
> > > >
> > > > struct report_list {
> > > > [...]
> > > > struct report reports[];
> > > > } __packed;
> >
> > Therefore, the "struct hostif_msg" is now made up of a header and a
> > payload. And the "struct report" uses only the "hostif_msg" header.
> > The perfect solution would be for the "report" structure to use the
> > whole "hostif_msg" structure but this is not possible due to nested
> > flexible arrays. Anyway, the end result is equivalent since this
> > patch
> > does attemp to change the behaviour of the code.
> >
> > Now as well, we have more clarity after the cast from the raw bytes
> > to
> > the new structures.
> >
> > > >
> > > > Also, use "container_of()" whenever we need to retrieve a pointer
> > > > to
> > > > the flexible structure, through which we can access the flexible
> > > > array
> > > > if needed.
> >
> > I would like to know if it is enough :)
>
> The apporoach is fine. But I don't like clubbing other changes like
> struct_size(). That make code difficult to follow.
Erick, can you respin this patch without the struct_size() change? I
think it looks like it could land otherwise.
-Kees
>
> Thanks,
> Srinivas
>
>
>
> >
> > Regards,
> > Erick
> > >
> > > Thanks,
> > > Srinivas
>
--
Kees Cook
