Received-SPF: pass (google.com: domain of linux-kernel+bounces-206953-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <80976997acb82fe3e6ba54fa2708c8f40fb3eb00.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>, 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>
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-hardening@vger.kernel.org
Date: Sat, 08 Jun 2024 01:42:54 -0700
In-Reply-To: <AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com>
References: 
	<AS8PR02MB723760CB93942370E92F00638BF72@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 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.

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.

Thanks,
Srinivas

>=20
> Link:
> https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-=
and-one-element-arrays
> =C2=A0[1]
> Closes: https://github.com/KSPP/linux/issues/333
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
> ---
> Hi,
>=20
> The idea behind this patch is extracted from the ones sent by Gustavo
> A. R. Silva [1] but without the use of "struct_group_tagged()" helper
> to separate the flexible array from the rest of the members in the
> flexible structures.
>=20
> Regarding adding the "__counted_by" attribute to the flexible arrays,
> I can say that I have not dared. The reasons are:
>=20
> 1.- In both arrays there are a no direct assignment to the counter
> =C2=A0=C2=A0=C2=A0 member. Only exists a cast from a raw stream of bytes =
to a
> pointer
> =C2=A0=C2=A0=C2=A0 of a structure and this way the counter member has the=
 value.
>=20
> 2.- The outer flexible array (in the struct report_list) has elements
> =C2=A0=C2=A0=C2=A0 of different size. I believe that every report can hav=
e a
> different
> =C2=A0=C2=A0=C2=A0 size, so I think the "__counted_by" will not work as e=
xpected.
>=20
> Comments are welcome ;)
>=20
> Regards,
> Erick
>=20
> [1] Here are some patches that use the same idea:
> Link:
> https://lore.kernel.org/linux-hardening/cover.1709658886.git.gustavoars@k=
ernel.org/
> Link: https://lore.kernel.org/linux-hardening/ZgYWlkxdrrieDYIu@neat/
> Link: https://lore.kernel.org/linux-hardening/ZgG8bbEzhmX5nGRE@neat/
> ---
> =C2=A0drivers/hid/intel-ish-hid/ishtp-hid-client.c | 27 ++++++++++-------=
-
> --
> =C2=A0drivers/hid/intel-ish-hid/ishtp-hid.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 | 11 +++++---
> =C2=A02 files changed, 20 insertions(+), 18 deletions(-)
>=20
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index fbd4f8ea1951..c0c8f4d7b0e3 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -70,10 +70,10 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> =C2=A0	unsigned char *payload;
> =C2=A0	struct device_info *dev_info;
> =C2=A0	int i, j;
> -	size_t	payload_len, total_len, cur_pos, raw_len;
> +	size_t	payload_len, total_len, cur_pos, raw_len, msg_len;
> =C2=A0	int report_type;
> =C2=A0	struct report_list *reports_list;
> -	char *reports;
> +	struct report *report;
> =C2=A0	size_t report_len;
> =C2=A0	struct ishtp_cl_data *client_data =3D
> ishtp_get_client_data(hid_ishtp_cl);
> =C2=A0	int curr_hid_dev =3D client_data->cur_hid_dev;
> @@ -99,7 +99,7 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> =C2=A0		payload_len =3D recv_msg->hdr.size;
> =C2=A0
> =C2=A0		/* Sanity checks */
> -		if (cur_pos + payload_len + sizeof(struct
> hostif_msg) >
> +		if (cur_pos + struct_size(recv_msg, payload,
> payload_len) >
> =C2=A0				total_len) {
> =C2=A0			++client_data->bad_recv_cnt;
> =C2=A0			report_bad_packet(hid_ishtp_cl, recv_msg,
> cur_pos,
> @@ -280,14 +280,13 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> =C2=A0		case HOSTIF_PUBLISH_INPUT_REPORT_LIST:
> =C2=A0			report_type =3D HID_INPUT_REPORT;
> =C2=A0			reports_list =3D (struct report_list
> *)payload;
> -			reports =3D (char *)reports_list->reports;
> +			report =3D reports_list->reports;
> =C2=A0
> =C2=A0			for (j =3D 0; j < reports_list-
> >num_of_reports; j++) {
> -				recv_msg =3D (struct hostif_msg
> *)(reports +
> -					sizeof(uint16_t));
> -				report_len =3D *(uint16_t *)reports;
> -				payload =3D reports + sizeof(uint16_t)
> +
> -					sizeof(struct
> hostif_msg_hdr);
> +				recv_msg =3D container_of(&report-
> >msg,
> +							struct
> hostif_msg, hdr);
> +				report_len =3D report->size;
> +				payload =3D recv_msg->payload;
> =C2=A0				payload_len =3D report_len -
> =C2=A0					sizeof(struct
> hostif_msg_hdr);
> =C2=A0
> @@ -304,7 +303,7 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> =C2=A0						0);
> =C2=A0					}
> =C2=A0
> -				reports +=3D sizeof(uint16_t) +
> report_len;
> +				report +=3D sizeof(*report) +
> payload_len;
> =C2=A0			}
> =C2=A0			break;
> =C2=A0		default:
> @@ -316,12 +315,12 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> =C2=A0
> =C2=A0		}
> =C2=A0
> -		if (!cur_pos && cur_pos + payload_len +
> -				sizeof(struct hostif_msg) <
> total_len)
> +		msg_len =3D struct_size(recv_msg, payload,
> payload_len);
> +		if (!cur_pos && cur_pos + msg_len < total_len)
> =C2=A0			++client_data->multi_packet_cnt;
> =C2=A0
> -		cur_pos +=3D payload_len + sizeof(struct hostif_msg);
> -		payload +=3D payload_len + sizeof(struct hostif_msg);
> +		cur_pos +=3D msg_len;
> +		payload +=3D msg_len;
> =C2=A0
> =C2=A0	} while (cur_pos < total_len);
> =C2=A0}
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.h
> b/drivers/hid/intel-ish-hid/ishtp-hid.h
> index 35dddc5015b3..2bc19e8ba13e 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid.h
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid.h
> @@ -31,6 +31,7 @@ struct hostif_msg_hdr {
> =C2=A0
> =C2=A0struct hostif_msg {
> =C2=A0	struct hostif_msg_hdr	hdr;
> +	uint8_t payload[];
> =C2=A0} __packed;
> =C2=A0
> =C2=A0struct hostif_msg_to_sensor {
> @@ -52,15 +53,17 @@ struct ishtp_version {
> =C2=A0	uint16_t build;
> =C2=A0} __packed;
> =C2=A0
> +struct report {
> +	uint16_t size;
> +	struct hostif_msg_hdr msg;
> +} __packed;
> +
> =C2=A0/* struct for ISHTP aggregated input data */
> =C2=A0struct report_list {
> =C2=A0	uint16_t total_size;
> =C2=A0	uint8_t	num_of_reports;
> =C2=A0	uint8_t	flags;
> -	struct {
> -		uint16_t	size_of_report;
> -		uint8_t report[1];
> -	} __packed reports[1];
> +	struct report reports[];
> =C2=A0} __packed;
> =C2=A0
> =C2=A0/* HOSTIF commands */