linux-kernel - Re: [RFC] HID: ishtp-hid-client: replace fake-flex arrays with flex-array 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]

Date: Sat, 8 Jun 2024 11:56:23 +0200
From: Erick Archer <erick.archer@...look.com>
To: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc: Erick Archer <erick.archer@...look.com>, Jiri Kosina <jikos@...nel.org>,
	Benjamin Tissoires <bentiss@...nel.org>,
	Justin Stitt <justinstitt@...gle.com>,
	Kees Cook <keescook@...omium.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [RFC] HID: ishtp-hid-client: replace fake-flex arrays with
 flex-array members

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 :)

Regards,
Erick
> 
> Thanks,
> Srinivas