| Message ID | Y00JenqCzKRrcTiF@mail.google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 542f25a94471570e2594be5b422b9ca572cf88a1 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [next] HID: hyperv: Replace one-element array with flexible-array member | expand |
On Mon, Oct 17, 2022 at 9:51 AM Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > One-element arrays are deprecated, and we are replacing them with > flexible array members instead. So, replace one-element array with > flexible-array member in structs synthhid_msg, synthhid_input_report, > pipe_prt_msg and refactor the rest of the code accordingly. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/210 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > --- FWIW, this is Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Couple of questions though: - do the hyperv want to take this patch through their tree or should I (I plan on sending some fixes for 6.1-rc2 this week, so I can piggy back this one) - on the driver itself, please see inline: > drivers/hid/hid-hyperv.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c > index e0bc73124196..208cf8d981a5 100644 > --- a/drivers/hid/hid-hyperv.c > +++ b/drivers/hid/hid-hyperv.c > @@ -61,7 +61,7 @@ struct synthhid_msg_hdr { > > struct synthhid_msg { > struct synthhid_msg_hdr header; > - char data[1]; /* Enclosed message */ > + char data[]; /* Enclosed message */ > }; IMO that struct has no real use. We just use it in mousevsc_on_receive() to dereference the first field only, the header. So how about we have a followup cleanup patch that just removes it and in mousevsc_on_receive() we convert those usages directly to struct synthhid_msg_hdr? Cheers, Benjamin > > union synthhid_version { > @@ -99,7 +99,7 @@ struct synthhid_device_info_ack { > > struct synthhid_input_report { > struct synthhid_msg_hdr header; > - char buffer[1]; > + char buffer[]; > }; > > #pragma pack(pop) > @@ -118,7 +118,7 @@ enum pipe_prot_msg_type { > struct pipe_prt_msg { > enum pipe_prot_msg_type type; > u32 size; > - char data[1]; > + char data[]; > }; > > struct mousevsc_prt_msg { > @@ -232,7 +232,7 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device, > > ret = vmbus_sendpacket(input_device->device->channel, > &ack, > - sizeof(struct pipe_prt_msg) - sizeof(unsigned char) + > + sizeof(struct pipe_prt_msg) + > sizeof(struct synthhid_device_info_ack), > (unsigned long)&ack, > VM_PKT_DATA_INBAND, > @@ -271,16 +271,14 @@ static void mousevsc_on_receive(struct hv_device *device, > * malicious/buggy hypervisor/host, add a check here to > * ensure we don't corrupt memory. > */ > - if ((pipe_msg->size + sizeof(struct pipe_prt_msg) > - - sizeof(unsigned char)) > + if (struct_size(pipe_msg, data, pipe_msg->size) > > sizeof(struct mousevsc_prt_msg)) { > WARN_ON(1); > break; > } > > memcpy(&input_dev->protocol_resp, pipe_msg, > - pipe_msg->size + sizeof(struct pipe_prt_msg) - > - sizeof(unsigned char)); > + struct_size(pipe_msg, data, pipe_msg->size)); > complete(&input_dev->wait_event); > break; > > @@ -359,8 +357,7 @@ static int mousevsc_connect_to_vsp(struct hv_device *device) > request->request.version_requested.version = SYNTHHID_INPUT_VERSION; > > ret = vmbus_sendpacket(device->channel, request, > - sizeof(struct pipe_prt_msg) - > - sizeof(unsigned char) + > + sizeof(struct pipe_prt_msg) + > sizeof(struct synthhid_protocol_request), > (unsigned long)request, > VM_PKT_DATA_INBAND, > -- > 2.37.3 >
On Mon, Oct 17, 2022 at 10:30:54AM +0200, Benjamin Tissoires wrote: > On Mon, Oct 17, 2022 at 9:51 AM Paulo Miguel Almeida > <paulo.miguel.almeida.rodenas@gmail.com> wrote: > > > > One-element arrays are deprecated, and we are replacing them with > > flexible array members instead. So, replace one-element array with > > flexible-array member in structs synthhid_msg, synthhid_input_report, > > pipe_prt_msg and refactor the rest of the code accordingly. > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > routines on memcpy() and help us make progress towards globally > > enabling -fstrict-flex-arrays=3 [1]. > > > > Link: https://github.com/KSPP/linux/issues/79 > > Link: https://github.com/KSPP/linux/issues/210 > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > > --- > > FWIW, this is > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Thanks for reviewing this patch :-) > > +++ b/drivers/hid/hid-hyperv.c > > @@ -61,7 +61,7 @@ struct synthhid_msg_hdr { > > > > struct synthhid_msg { > > struct synthhid_msg_hdr header; > > - char data[1]; /* Enclosed message */ > > + char data[]; /* Enclosed message */ > > }; > > IMO that struct has no real use. We just use it in > mousevsc_on_receive() to dereference the first field only, the header. > So how about we have a followup cleanup patch that just removes it and > in mousevsc_on_receive() we convert those usages directly to struct > synthhid_msg_hdr? > > Cheers, > Benjamin I'm happy to send a followup cleanup patch for that. Paulo A.
From: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> Sent: Monday, October 17, 2022 12:51 AM > > One-element arrays are deprecated, and we are replacing them with > flexible array members instead. So, replace one-element array with > flexible-array member in structs synthhid_msg, synthhid_input_report, > pipe_prt_msg and refactor the rest of the code accordingly. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/210 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > --- > drivers/hid/hid-hyperv.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c > index e0bc73124196..208cf8d981a5 100644 > --- a/drivers/hid/hid-hyperv.c > +++ b/drivers/hid/hid-hyperv.c > @@ -61,7 +61,7 @@ struct synthhid_msg_hdr { > > struct synthhid_msg { > struct synthhid_msg_hdr header; > - char data[1]; /* Enclosed message */ > + char data[]; /* Enclosed message */ > }; > > union synthhid_version { > @@ -99,7 +99,7 @@ struct synthhid_device_info_ack { > > struct synthhid_input_report { > struct synthhid_msg_hdr header; > - char buffer[1]; > + char buffer[]; > }; > > #pragma pack(pop) > @@ -118,7 +118,7 @@ enum pipe_prot_msg_type { > struct pipe_prt_msg { > enum pipe_prot_msg_type type; > u32 size; > - char data[1]; > + char data[]; > }; > > struct mousevsc_prt_msg { > @@ -232,7 +232,7 @@ static void mousevsc_on_receive_device_info(struct > mousevsc_dev *input_device, > > ret = vmbus_sendpacket(input_device->device->channel, > &ack, > - sizeof(struct pipe_prt_msg) - sizeof(unsigned char) + > + sizeof(struct pipe_prt_msg) + > sizeof(struct synthhid_device_info_ack), > (unsigned long)&ack, > VM_PKT_DATA_INBAND, > @@ -271,16 +271,14 @@ static void mousevsc_on_receive(struct hv_device *device, > * malicious/buggy hypervisor/host, add a check here to > * ensure we don't corrupt memory. > */ > - if ((pipe_msg->size + sizeof(struct pipe_prt_msg) > - - sizeof(unsigned char)) > + if (struct_size(pipe_msg, data, pipe_msg->size) > > sizeof(struct mousevsc_prt_msg)) { > WARN_ON(1); > break; > } > > memcpy(&input_dev->protocol_resp, pipe_msg, > - pipe_msg->size + sizeof(struct pipe_prt_msg) - > - sizeof(unsigned char)); > + struct_size(pipe_msg, data, pipe_msg->size)); > complete(&input_dev->wait_event); > break; > > @@ -359,8 +357,7 @@ static int mousevsc_connect_to_vsp(struct hv_device *device) > request->request.version_requested.version = SYNTHHID_INPUT_VERSION; > > ret = vmbus_sendpacket(device->channel, request, > - sizeof(struct pipe_prt_msg) - > - sizeof(unsigned char) + > + sizeof(struct pipe_prt_msg) + > sizeof(struct synthhid_protocol_request), > (unsigned long)request, > VM_PKT_DATA_INBAND, > -- > 2.37.3 Reviewed-by: Michael Kelley <mikelley@microsoft.com>
On Mon, 17 Oct 2022, Michael Kelley (LINUX) wrote: > > One-element arrays are deprecated, and we are replacing them with > > flexible array members instead. So, replace one-element array with > > flexible-array member in structs synthhid_msg, synthhid_input_report, > > pipe_prt_msg and refactor the rest of the code accordingly. > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > routines on memcpy() and help us make progress towards globally > > enabling -fstrict-flex-arrays=3 [1]. > > > > Link: https://github.com/KSPP/linux/issues/79 > > Link: https://github.com/KSPP/linux/issues/210 > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > > --- > > drivers/hid/hid-hyperv.c | 17 +++++++---------- > > 1 file changed, 7 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c > > index e0bc73124196..208cf8d981a5 100644 > > --- a/drivers/hid/hid-hyperv.c > > +++ b/drivers/hid/hid-hyperv.c > > @@ -61,7 +61,7 @@ struct synthhid_msg_hdr { > > > > struct synthhid_msg { > > struct synthhid_msg_hdr header; > > - char data[1]; /* Enclosed message */ > > + char data[]; /* Enclosed message */ > > }; > > > > union synthhid_version { > > @@ -99,7 +99,7 @@ struct synthhid_device_info_ack { > > > > struct synthhid_input_report { > > struct synthhid_msg_hdr header; > > - char buffer[1]; > > + char buffer[]; > > }; > > > > #pragma pack(pop) > > @@ -118,7 +118,7 @@ enum pipe_prot_msg_type { > > struct pipe_prt_msg { > > enum pipe_prot_msg_type type; > > u32 size; > > - char data[1]; > > + char data[]; > > }; > > > > struct mousevsc_prt_msg { > > @@ -232,7 +232,7 @@ static void mousevsc_on_receive_device_info(struct > > mousevsc_dev *input_device, > > > > ret = vmbus_sendpacket(input_device->device->channel, > > &ack, > > - sizeof(struct pipe_prt_msg) - sizeof(unsigned char) + > > + sizeof(struct pipe_prt_msg) + > > sizeof(struct synthhid_device_info_ack), > > (unsigned long)&ack, > > VM_PKT_DATA_INBAND, > > @@ -271,16 +271,14 @@ static void mousevsc_on_receive(struct hv_device *device, > > * malicious/buggy hypervisor/host, add a check here to > > * ensure we don't corrupt memory. > > */ > > - if ((pipe_msg->size + sizeof(struct pipe_prt_msg) > > - - sizeof(unsigned char)) > > + if (struct_size(pipe_msg, data, pipe_msg->size) > > > sizeof(struct mousevsc_prt_msg)) { > > WARN_ON(1); > > break; > > } > > > > memcpy(&input_dev->protocol_resp, pipe_msg, > > - pipe_msg->size + sizeof(struct pipe_prt_msg) - > > - sizeof(unsigned char)); > > + struct_size(pipe_msg, data, pipe_msg->size)); > > complete(&input_dev->wait_event); > > break; > > > > @@ -359,8 +357,7 @@ static int mousevsc_connect_to_vsp(struct hv_device *device) > > request->request.version_requested.version = SYNTHHID_INPUT_VERSION; > > > > ret = vmbus_sendpacket(device->channel, request, > > - sizeof(struct pipe_prt_msg) - > > - sizeof(unsigned char) + > > + sizeof(struct pipe_prt_msg) + > > sizeof(struct synthhid_protocol_request), > > (unsigned long)request, > > VM_PKT_DATA_INBAND, > > -- > > 2.37.3 > > Reviewed-by: Michael Kelley <mikelley@microsoft.com> Thanks, applied.
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c index e0bc73124196..208cf8d981a5 100644 --- a/drivers/hid/hid-hyperv.c +++ b/drivers/hid/hid-hyperv.c @@ -61,7 +61,7 @@ struct synthhid_msg_hdr { struct synthhid_msg { struct synthhid_msg_hdr header; - char data[1]; /* Enclosed message */ + char data[]; /* Enclosed message */ }; union synthhid_version { @@ -99,7 +99,7 @@ struct synthhid_device_info_ack { struct synthhid_input_report { struct synthhid_msg_hdr header; - char buffer[1]; + char buffer[]; }; #pragma pack(pop) @@ -118,7 +118,7 @@ enum pipe_prot_msg_type { struct pipe_prt_msg { enum pipe_prot_msg_type type; u32 size; - char data[1]; + char data[]; }; struct mousevsc_prt_msg { @@ -232,7 +232,7 @@ static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device, ret = vmbus_sendpacket(input_device->device->channel, &ack, - sizeof(struct pipe_prt_msg) - sizeof(unsigned char) + + sizeof(struct pipe_prt_msg) + sizeof(struct synthhid_device_info_ack), (unsigned long)&ack, VM_PKT_DATA_INBAND, @@ -271,16 +271,14 @@ static void mousevsc_on_receive(struct hv_device *device, * malicious/buggy hypervisor/host, add a check here to * ensure we don't corrupt memory. */ - if ((pipe_msg->size + sizeof(struct pipe_prt_msg) - - sizeof(unsigned char)) + if (struct_size(pipe_msg, data, pipe_msg->size) > sizeof(struct mousevsc_prt_msg)) { WARN_ON(1); break; } memcpy(&input_dev->protocol_resp, pipe_msg, - pipe_msg->size + sizeof(struct pipe_prt_msg) - - sizeof(unsigned char)); + struct_size(pipe_msg, data, pipe_msg->size)); complete(&input_dev->wait_event); break; @@ -359,8 +357,7 @@ static int mousevsc_connect_to_vsp(struct hv_device *device) request->request.version_requested.version = SYNTHHID_INPUT_VERSION; ret = vmbus_sendpacket(device->channel, request, - sizeof(struct pipe_prt_msg) - - sizeof(unsigned char) + + sizeof(struct pipe_prt_msg) + sizeof(struct synthhid_protocol_request), (unsigned long)request, VM_PKT_DATA_INBAND,
One-element arrays are deprecated, and we are replacing them with flexible array members instead. So, replace one-element array with flexible-array member in structs synthhid_msg, synthhid_input_report, pipe_prt_msg and refactor the rest of the code accordingly. This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy() and help us make progress towards globally enabling -fstrict-flex-arrays=3 [1]. Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/210 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> --- drivers/hid/hid-hyperv.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)