Received-SPF: pass (google.com: domain of linux-kernel+bounces-210975-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4e68c0164022ca41494c6d577766dd4b66c93e9f.camel@linux.intel.com>
Subject: Re: [RFC] HID: ishtp-hid-client: replace fake-flex arrays with
 flex-array members
From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
To: Erick Archer <erick.archer@outlook.com>
Cc: Jiri Kosina <jikos@kernel.org>, Benjamin Tissoires <bentiss@kernel.org>,
  Justin Stitt <justinstitt@google.com>, Kees Cook <keescook@chromium.org>,
 "Gustavo A. R. Silva" <gustavoars@kernel.org>, linux-input@vger.kernel.org,
  linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Date: Tue, 11 Jun 2024 23:26:25 -0700
In-Reply-To: <AS8PR02MB723798FF6CEF28DCB62FAC958BC42@AS8PR02MB7237.eurprd02.prod.outlook.com>
References: 
	<AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com>
	 <80976997acb82fe3e6ba54fa2708c8f40fb3eb00.camel@linux.intel.com>
	 <AS8PR02MB723798FF6CEF28DCB62FAC958BC42@AS8PR02MB7237.eurprd02.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.0-1 
Precedence: bulk
MIME-Version: 1.0

On Sat, 2024-06-08 at 11:56 +0200, Erick Archer wrote:
> Hi Srinivas,
> First of all, thanks for looking at this ;)
>=20
> 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.
> > >=20
> > > So, define a new struct type for the single reports:
> > >=20
> > > struct report {
> > > 	uint16_t size;
> > > 	struct hostif_msg_hdr msg;
> > > } __packed;
> > >=20
> > > 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.
> > >=20
> > > struct report_list {
> > > 	[...]
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct report reports[];
> > > } __packed;
> > >=20
> > > 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.
> > >=20
> > > 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.
> > >=20
> > > This way, the code is more readable and safer.
> >=20
> > Applied and tested, atleast didn't break anything.
> >=20
> > But the explanation above didn't give me enough clue. You have
> > added a
> > payload[] in the=C2=A0 struct hostif_msg {} then using that as a messag=
e
> > pointer following the header. I think this description needs to be
> > better.
>=20
> Yeah, I will try to improve the commit message. What do you think
> about
> the following parragrafs?
>=20
> [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.
> > >=20
> > > struct report_list {
> > > 	[...]
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct report reports[];
> > > } __packed;
>=20
> 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.
>=20
> Now as well, we have more clarity after the cast from the raw bytes
> to
> the new structures.
>=20
> > >=20
> > > 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.
>=20
> 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.

Thanks,
Srinivas


>=20
> Regards,
> Erick
> >=20
> > Thanks,
> > Srinivas