[v2] HID: ishtp-hid-client: replace fake-flex arrays with flex-array members

Commit Message

Kees Cook Sept. 23, 2024, 12:22 a.m. UTC

From: Erick Archer <[e-mail]>

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 <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
[kees: tweaked commit log and dropped struct_size() uses]
Signed-off-by: Kees Cook <[e-mail]>
---
 v2: - update based on feedback
 rfc: https://lore.kernel.org/lkml/[e-mail]/
---
 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(-)

Comments

Srinivas Pandruvada Sept. 23, 2024, 7:32 p.m. UTC | #1

On Sun, 2024-09-22 at 17:22 -0700, Kees Cook wrote:
> From: Erick Archer <[e-mail]>
> 
> 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 <[e-mail]>
> Link:
> https://lore.kernel.org/r/[e-mail]
> [kees: tweaked commit log and dropped struct_size() uses]
> Signed-off-by: Kees Cook <[e-mail]>
Acked-by: Srinivas Pandruvada <[e-mail]>

> ---
>  v2: - update based on feedback
>  rfc:
> https://lore.kernel.org/lkml/[e-mail]/
> ---
>  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 */

Jiri Kosina Oct. 8, 2024, 6:52 a.m. UTC | #2

On Sun, 22 Sep 2024, Kees Cook wrote:

> From: Erick Archer <[e-mail]>
> 
> 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 <[e-mail]>
> Link: https://lore.kernel.org/r/[e-mail]
> [kees: tweaked commit log and dropped struct_size() uses]
> Signed-off-by: Kees Cook <[e-mail]>
> ---
>  v2: - update based on feedback
>  rfc: https://lore.kernel.org/lkml/[e-mail]/

Applied, thanks.

Patch

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 */