| Message ID | AS8PR02MB723760CB93942370E92F00638BF72@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] HID: ishtp-hid-client: replace fake-flex arrays with flex-array members | expand |
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@outlook.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 */
Hi Srinivas, First of all, thanks for looking at this ;) On Sat, Jun 08, 2024 at 01:42:54AM -0700, srinivas pandruvada wrote: > 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. Yeah, I will try to improve the commit message. What do you think about the following parragrafs? [I have copied part of the message to show where the new info will be] > > 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 attemp 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. > > > > 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. I would like to know if it is enough :) Regards, Erick > > Thanks, > Srinivas
On Sat, 2024-06-08 at 11:56 +0200, Erick Archer wrote: > Hi Srinivas, > First of all, thanks for looking at this ;) > > On Sat, Jun 08, 2024 at 01:42:54AM -0700, srinivas pandruvada wrote: > > 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. > > Yeah, I will try to improve the commit message. What do you think > about > the following parragrafs? > > [I have copied part of the message to show where the new info will > be] > > > 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 attemp 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. > > > > > > > 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. > > I would like to know if it is enough :) The apporoach is fine. But I don't like clubbing other changes like struct_size(). That make code difficult to follow. Thanks, Srinivas > > Regards, > Erick > > > > Thanks, > > Srinivas
On Tue, Jun 11, 2024 at 11:26:25PM -0700, srinivas pandruvada wrote: > On Sat, 2024-06-08 at 11:56 +0200, Erick Archer wrote: > > Hi Srinivas, > > First of all, thanks for looking at this ;) > > > > On Sat, Jun 08, 2024 at 01:42:54AM -0700, srinivas pandruvada wrote: > > > 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. > > > > Yeah, I will try to improve the commit message. What do you think > > about > > the following parragrafs? > > > > [I have copied part of the message to show where the new info will > > be] > > > > 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 attemp 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. > > > > > > > > > > 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. > > > > I would like to know if it is enough :) > > The apporoach is fine. But I don't like clubbing other changes like > struct_size(). That make code difficult to follow. Erick, can you respin this patch without the struct_size() change? I think it looks like it could land otherwise. -Kees > > Thanks, > Srinivas > > > > > > > Regards, > > Erick > > > > > > Thanks, > > > Srinivas >
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 */
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. 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@outlook.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(-)