| 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
| ||
|
Message-ID: <80976997acb82fe3e6ba54fa2708c8f40fb3eb00.camel@linux.intel.com>
Date: Sat, 08 Jun 2024 01:42:54 -0700
From: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
To: 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>
Cc: 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
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.
Thanks,
Srinivas
>
> Link:
> https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays
> [1]
> Closes: https://github.com/KSPP/linux/issues/333
> Signed-off-by: Erick Archer <erick.archer@...look.com>
> ---
> Hi,
>
> 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.
>
> Regarding adding the "__counted_by" attribute to the flexible arrays,
> I can say that I have not dared. The reasons are:
>
> 1.- In both arrays there are a no direct assignment to the counter
> member. Only exists a cast from a raw stream of bytes to a
> pointer
> of a structure and this way the counter member has the value.
>
> 2.- The outer flexible array (in the struct report_list) has elements
> of different size. I believe that every report can have a
> different
> size, so I think the "__counted_by" will not work as expected.
>
> Comments are welcome ;)
>
> Regards,
> Erick
>
> [1] Here are some patches that use the same idea:
> Link:
> https://lore.kernel.org/linux-hardening/cover.1709658886.git.gustavoars@kernel.org/
> Link: https://lore.kernel.org/linux-hardening/ZgYWlkxdrrieDYIu@neat/
> Link: https://lore.kernel.org/linux-hardening/ZgG8bbEzhmX5nGRE@neat/
> ---
> drivers/hid/intel-ish-hid/ishtp-hid-client.c | 27 ++++++++++--------
> --
> drivers/hid/intel-ish-hid/ishtp-hid.h | 11 +++++---
> 2 files changed, 20 insertions(+), 18 deletions(-)
>
> 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,
> unsigned char *payload;
> struct device_info *dev_info;
> 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;
> int report_type;
> struct report_list *reports_list;
> - char *reports;
> + struct report *report;
> size_t report_len;
> struct ishtp_cl_data *client_data =
> ishtp_get_client_data(hid_ishtp_cl);
> int curr_hid_dev = client_data->cur_hid_dev;
> @@ -99,7 +99,7 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> payload_len = recv_msg->hdr.size;
>
> /* Sanity checks */
> - if (cur_pos + payload_len + sizeof(struct
> hostif_msg) >
> + if (cur_pos + struct_size(recv_msg, payload,
> payload_len) >
> total_len) {
> ++client_data->bad_recv_cnt;
> 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,
> case HOSTIF_PUBLISH_INPUT_REPORT_LIST:
> report_type = HID_INPUT_REPORT;
> reports_list = (struct report_list
> *)payload;
> - reports = (char *)reports_list->reports;
> + report = reports_list->reports;
>
> for (j = 0; j < reports_list-
> >num_of_reports; j++) {
> - recv_msg = (struct hostif_msg
> *)(reports +
> - sizeof(uint16_t));
> - report_len = *(uint16_t *)reports;
> - payload = reports + sizeof(uint16_t)
> +
> - sizeof(struct
> hostif_msg_hdr);
> + recv_msg = container_of(&report-
> >msg,
> + struct
> hostif_msg, hdr);
> + report_len = report->size;
> + payload = recv_msg->payload;
> payload_len = report_len -
> sizeof(struct
> hostif_msg_hdr);
>
> @@ -304,7 +303,7 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
> 0);
> }
>
> - reports += sizeof(uint16_t) +
> report_len;
> + report += sizeof(*report) +
> payload_len;
> }
> break;
> default:
> @@ -316,12 +315,12 @@ static void process_recv(struct ishtp_cl
> *hid_ishtp_cl, void *recv_buf,
>
> }
>
> - if (!cur_pos && cur_pos + payload_len +
> - sizeof(struct hostif_msg) <
> total_len)
> + msg_len = struct_size(recv_msg, payload,
> payload_len);
> + if (!cur_pos && cur_pos + msg_len < total_len)
> ++client_data->multi_packet_cnt;
>
> - cur_pos += payload_len + sizeof(struct hostif_msg);
> - payload += payload_len + sizeof(struct hostif_msg);
> + cur_pos += msg_len;
> + payload += msg_len;
>
> } while (cur_pos < total_len);
> }
> 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 {
>
> struct hostif_msg {
> struct hostif_msg_hdr hdr;
> + uint8_t payload[];
> } __packed;
>
> struct hostif_msg_to_sensor {
> @@ -52,15 +53,17 @@ struct ishtp_version {
> uint16_t build;
> } __packed;
>
> +struct report {
> + uint16_t size;
> + struct hostif_msg_hdr msg;
> +} __packed;
> +
> /* struct for ISHTP aggregated input data */
> struct report_list {
> uint16_t total_size;
> uint8_t num_of_reports;
> uint8_t flags;
> - struct {
> - uint16_t size_of_report;
> - uint8_t report[1];
> - } __packed reports[1];
> + struct report reports[];
> } __packed;
>
> /* HOSTIF commands */
Powered by blists - more mailing lists