| 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: <c7c2665bf433a1648a3f7dff20ac9976ef1defe3.camel@linux.intel.com>
Date: Mon, 23 Sep 2024 12:32:37 -0700
From: srinivas pandruvada <srinivas.pandruvada@...ux.intel.com>
To: Kees Cook <kees@...nel.org>
Cc: Erick Archer <erick.archer@...look.com>, Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <bentiss@...nel.org>, linux-kernel@...r.kernel.org,
linux-input@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] HID: ishtp-hid-client: replace fake-flex arrays with
flex-array members
On Sun, 2024-09-22 at 17:22 -0700, Kees Cook wrote:
> From: Erick Archer <erick.archer@...look.com>
>
> One-element arrays as fake flex arrays are deprecated[1] as the
> kernel
> has switched to C99 flexible-array members instead. This case,
> however,
> 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;
>
> 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 attempt 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. Refactor the code accordingly to use 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.
>
> 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>
> Link:
> https://lore.kernel.org/r/AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com
> [kees: tweaked commit log and dropped struct_size() uses]
> Signed-off-by: Kees Cook <kees@...nel.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> ---
> v2: - update based on feedback
> rfc:
> https://lore.kernel.org/lkml/AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com/
> ---
> drivers/hid/intel-ish-hid/ishtp-hid-client.c | 25 ++++++++++--------
> --
> drivers/hid/intel-ish-hid/ishtp-hid.h | 11 +++++----
> 2 files changed, 19 insertions(+), 17 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..cb04cd1d980b 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;
> @@ -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 = payload_len + sizeof(struct hostif_msg);
> + 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