diff mbox series

[next] HID: hyperv: Replace one-element array with flexible-array member

Message ID Y00JenqCzKRrcTiF@mail.google.com
State New
Headers
Series [next] HID: hyperv: Replace one-element array with flexible-array member |

Commit Message

Paulo Miguel Almeida Oct. 17, 2022, 7:51 a.m. UTC 
  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(-)

Comments

Benjamin Tissoires Oct. 17, 2022, 8:30 a.m. UTC | #1 
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
>
Paulo Miguel Almeida Oct. 17, 2022, 9:30 a.m. UTC | #2 
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.
Michael Kelley (LINUX) Oct. 17, 2022, 3:15 p.m. UTC | #3 
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>
Jiri Kosina Oct. 18, 2022, 12:56 p.m. UTC | #4 
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 mbox series

Patch

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,