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

[srinivas pandruvada]

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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx/
> 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 */