[PATCH] usbip: Remove unaligned pointer usage from usbip tools

[PATCH] usbip: Remove unaligned pointer usage from usbip tools

[Vadim Troshchinskiy]

The usbip tools use packed structs for network communication. Taking the
address of a packed member of a struct can crash the program with SIGBUS
on architectures with strict alignment requirements.

Also, recent versions of GCC detect this situation and emit a warning that
is fatal due to -Werror being used.

error: taking address of packed member of ‘struct
usbip_usb_device’ may result in an unaligned pointer value [-Werror=address-
of-packed-member]

Fix this by copying the data to an aligned location and operating there.

Signed-off-by: Vadim Troshchinskiy <vtroshchinskiy@qindel.com>
---
 tools/usb/usbip/src/usbip_network.c | 30 +++++++++++++++--------------
 tools/usb/usbip/src/usbip_network.h | 12 ++++++------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/tools/usb/usbip/src/usbip_network.c b/tools/usb/usbip/src/usbip_network.c
index d595d72693fb..1c0038ee0abd 100644
--- a/tools/usb/usbip/src/usbip_network.c
+++ b/tools/usb/usbip/src/usbip_network.c
@@ -50,39 +50,41 @@ void usbip_setup_port_number(char *arg)
 	info("using port %d (\"%s\")", usbip_port, usbip_port_string);
 }
 
-void usbip_net_pack_uint32_t(int pack, uint32_t *num)
+void usbip_net_pack_uint32_t(int pack, uint8_t *num)
 {
 	uint32_t i;
+	memcpy(&i, num, sizeof(i));
 
 	if (pack)
-		i = htonl(*num);
+		i = htonl(i);
 	else
-		i = ntohl(*num);
+		i = ntohl(i);
 
-	*num = i;
+	memcpy(num, &i, sizeof(i));
 }
 
-void usbip_net_pack_uint16_t(int pack, uint16_t *num)
+void usbip_net_pack_uint16_t(int pack, uint8_t *num)
 {
 	uint16_t i;
+	memcpy(&i, num, sizeof(i));
 
 	if (pack)
-		i = htons(*num);
+		i = htons(i);
 	else
-		i = ntohs(*num);
+		i = ntohs(i);
 
-	*num = i;
+	memcpy(num, &i, sizeof(i));
 }
 
 void usbip_net_pack_usb_device(int pack, struct usbip_usb_device *udev)
 {
-	usbip_net_pack_uint32_t(pack, &udev->busnum);
-	usbip_net_pack_uint32_t(pack, &udev->devnum);
-	usbip_net_pack_uint32_t(pack, &udev->speed);
+	usbip_net_pack_uint32_t(pack, (uint8_t*)&udev->busnum);
+	usbip_net_pack_uint32_t(pack, (uint8_t*)&udev->devnum);
+	usbip_net_pack_uint32_t(pack, (uint8_t*)&udev->speed);
 
-	usbip_net_pack_uint16_t(pack, &udev->idVendor);
-	usbip_net_pack_uint16_t(pack, &udev->idProduct);
-	usbip_net_pack_uint16_t(pack, &udev->bcdDevice);
+	usbip_net_pack_uint16_t(pack, (uint8_t*)&udev->idVendor);
+	usbip_net_pack_uint16_t(pack, (uint8_t*)&udev->idProduct);
+	usbip_net_pack_uint16_t(pack, (uint8_t*)&udev->bcdDevice);
 }
 
 void usbip_net_pack_usb_interface(int pack __attribute__((unused)),
diff --git a/tools/usb/usbip/src/usbip_network.h b/tools/usb/usbip/src/usbip_network.h
index 555215eae43e..821dd65877cc 100644
--- a/tools/usb/usbip/src/usbip_network.h
+++ b/tools/usb/usbip/src/usbip_network.h
@@ -33,9 +33,9 @@ struct op_common {
 } __attribute__((packed));
 
 #define PACK_OP_COMMON(pack, op_common)  do {\
-	usbip_net_pack_uint16_t(pack, &(op_common)->version);\
-	usbip_net_pack_uint16_t(pack, &(op_common)->code);\
-	usbip_net_pack_uint32_t(pack, &(op_common)->status);\
+	usbip_net_pack_uint16_t(pack, (uint8_t*)&(op_common)->version);\
+	usbip_net_pack_uint16_t(pack, (uint8_t*)&(op_common)->code);\
+	usbip_net_pack_uint32_t(pack, (uint8_t*)&(op_common)->status);\
 } while (0)
 
 /* ---------------------------------------------------------------------- */
@@ -163,11 +163,11 @@ struct op_devlist_reply_extra {
 } while (0)
 
 #define PACK_OP_DEVLIST_REPLY(pack, reply)  do {\
-	usbip_net_pack_uint32_t(pack, &(reply)->ndev);\
+	usbip_net_pack_uint32_t(pack, (uint8_t*)&(reply)->ndev);\
 } while (0)
 
-void usbip_net_pack_uint32_t(int pack, uint32_t *num);
-void usbip_net_pack_uint16_t(int pack, uint16_t *num);
+void usbip_net_pack_uint32_t(int pack, uint8_t *num);
+void usbip_net_pack_uint16_t(int pack, uint8_t *num);
 void usbip_net_pack_usb_device(int pack, struct usbip_usb_device *udev);
 void usbip_net_pack_usb_interface(int pack, struct usbip_usb_interface *uinf);
 
-- 
2.21.0

Re: [PATCH] usbip: Remove unaligned pointer usage from usbip tools

[shuah]

Hi Vadim,

On 12/10/19 8:50 AM, Vadim Troshchinskiy wrote:
> The usbip tools use packed structs for network communication. Taking the
> address of a packed member of a struct can crash the program with SIGBUS
> on architectures with strict alignment requirements.
> 

Can you be more specific on which architectures?

> Also, recent versions of GCC detect this situation and emit a warning that
> is fatal due to -Werror being used.
> 
> error: taking address of packed member of ‘struct
> usbip_usb_device’ may result in an unaligned pointer value [-Werror=address-
> of-packed-member]
> 
> Fix this by copying the data to an aligned location and operating there.
> 
> Signed-off-by: Vadim Troshchinskiy <vtroshchinskiy@qindel.com>
> ---
>   tools/usb/usbip/src/usbip_network.c | 30 +++++++++++++++--------------
>   tools/usb/usbip/src/usbip_network.h | 12 ++++++------
>   2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/usbip_network.c b/tools/usb/usbip/src/usbip_network.c
> index d595d72693fb..1c0038ee0abd 100644
> --- a/tools/usb/usbip/src/usbip_network.c
> +++ b/tools/usb/usbip/src/usbip_network.c
> @@ -50,39 +50,41 @@ void usbip_setup_port_number(char *arg)
>   	info("using port %d (\"%s\")", usbip_port, usbip_port_string);
>   }
>   
> -void usbip_net_pack_uint32_t(int pack, uint32_t *num)
> +void usbip_net_pack_uint32_t(int pack, uint8_t *num)

Is there a reason to change this to uint8_t?

>   {
>   	uint32_t i;
> +	memcpy(&i, num, sizeof(i));
>   
>   	if (pack)
> -		i = htonl(*num);
> +		i = htonl(i);
>   	else
> -		i = ntohl(*num);
> +		i = ntohl(i);
>   
> -	*num = i;
> +	memcpy(num, &i, sizeof(i));
>   }
>   
> -void usbip_net_pack_uint16_t(int pack, uint16_t *num)
> +void usbip_net_pack_uint16_t(int pack, uint8_t *num)

Is there a reason to change this to uint8_t?

>   {
>   	uint16_t i;
> +	memcpy(&i, num, sizeof(i));
>   
>   	if (pack)
> -		i = htons(*num);
> +		i = htons(i);
>   	else
> -		i = ntohs(*num);
> +		i = ntohs(i);
>   
> -	*num = i;
> +	memcpy(num, &i, sizeof(i));
>   }
>   
>   void usbip_net_pack_usb_device(int pack, struct usbip_usb_device *udev)
>   {
> -	usbip_net_pack_uint32_t(pack, &udev->busnum);
> -	usbip_net_pack_uint32_t(pack, &udev->devnum);
> -	usbip_net_pack_uint32_t(pack, &udev->speed);
> +	usbip_net_pack_uint32_t(pack, (uint8_t*)&udev->busnum);
> +	usbip_net_pack_uint32_t(pack, (uint8_t*)&udev->devnum);
> +	usbip_net_pack_uint32_t(pack, (uint8_t*)&udev->speed);
>   
> -	usbip_net_pack_uint16_t(pack, &udev->idVendor);
> -	usbip_net_pack_uint16_t(pack, &udev->idProduct);
> -	usbip_net_pack_uint16_t(pack, &udev->bcdDevice);
> +	usbip_net_pack_uint16_t(pack, (uint8_t*)&udev->idVendor);
> +	usbip_net_pack_uint16_t(pack, (uint8_t*)&udev->idProduct);
> +	usbip_net_pack_uint16_t(pack, (uint8_t*)&udev->bcdDevice);
>   }
>   
>   void usbip_net_pack_usb_interface(int pack __attribute__((unused)),
> diff --git a/tools/usb/usbip/src/usbip_network.h b/tools/usb/usbip/src/usbip_network.h
> index 555215eae43e..821dd65877cc 100644
> --- a/tools/usb/usbip/src/usbip_network.h
> +++ b/tools/usb/usbip/src/usbip_network.h
> @@ -33,9 +33,9 @@ struct op_common {
>   } __attribute__((packed));
>   
>   #define PACK_OP_COMMON(pack, op_common)  do {\
> -	usbip_net_pack_uint16_t(pack, &(op_common)->version);\
> -	usbip_net_pack_uint16_t(pack, &(op_common)->code);\
> -	usbip_net_pack_uint32_t(pack, &(op_common)->status);\
> +	usbip_net_pack_uint16_t(pack, (uint8_t*)&(op_common)->version);\
> +	usbip_net_pack_uint16_t(pack, (uint8_t*)&(op_common)->code);\
> +	usbip_net_pack_uint32_t(pack, (uint8_t*)&(op_common)->status);\
>   } while (0)
>   
>   /* ---------------------------------------------------------------------- */
> @@ -163,11 +163,11 @@ struct op_devlist_reply_extra {
>   } while (0)
>   
>   #define PACK_OP_DEVLIST_REPLY(pack, reply)  do {\
> -	usbip_net_pack_uint32_t(pack, &(reply)->ndev);\
> +	usbip_net_pack_uint32_t(pack, (uint8_t*)&(reply)->ndev);\
>   } while (0)
>   
> -void usbip_net_pack_uint32_t(int pack, uint32_t *num);
> -void usbip_net_pack_uint16_t(int pack, uint16_t *num);
> +void usbip_net_pack_uint32_t(int pack, uint8_t *num);
> +void usbip_net_pack_uint16_t(int pack, uint8_t *num);
>   void usbip_net_pack_usb_device(int pack, struct usbip_usb_device *udev);
>   void usbip_net_pack_usb_interface(int pack, struct usbip_usb_interface *uinf);
>   
> 

thanks,
-- Shuah

Re: [PATCH] usbip: Remove unaligned pointer usage from usbip tools

[Vadim Troshchinskiy]

----- Mensaje original -----
> De: "shuah" <shuah@kernel.org>
> Para: "Vadim Troshchinskiy" <vtroshchinskiy@qindel.com>, linux-usb@vger.kernel.org
> CC: "Valentina Manea" <valentina.manea.m@gmail.com>, "shuah" <shuah@kernel.org>
> Enviados: Jueves, 2 de Enero 2020 1:01:24
> Asunto: Re: [PATCH] usbip: Remove unaligned pointer usage from usbip tools
> 
> Hi Vadim,
> 


Hello! Sorry for the late reply, I was on vacation.


> On 12/10/19 8:50 AM, Vadim Troshchinskiy wrote:
> > The usbip tools use packed structs for network communication. Taking the
> > address of a packed member of a struct can crash the program with SIGBUS
> > on architectures with strict alignment requirements.
> > 
> 
> Can you be more specific on which architectures?


To my knowledge, older ARM processors, SPARC, probably others mostly of the low
power/embedded kinds. I personally work with x86, so this isn't a critical issue
for me, but I'm noting it's a portability concern.

On architectures which don't produce an error it's typically a significant
performance penalty (though it wouldn't be that big of a deal in this case in
particular)

In my case the main issue is that recent versions of GCC warn about this and
fail when -Werror is used. Sure, one could disable the warning or remove -Werror,
but that would obscure the issue and could cause hard to figure out problems
for somebody else down the line.



> >   
> > -void usbip_net_pack_uint16_t(int pack, uint16_t *num)
> > +void usbip_net_pack_uint16_t(int pack, uint8_t *num)
> 
> Is there a reason to change this to uint8_t?
> 


On architectures that require alignment, access must be done on multiples
of the data size. So for instance a 16 bit value must be located at byte
addresses 0, 2, 4, 6, etc, and a 32 bit value would have to be at addresses
0, 4, 8, etc.

So taking a pointer to a 16 bit value located at address 3 is a problem, but
accessing a byte at the same address is just fine.

malloc always aligns allocations to the maximum amount necessary for any
value on the architecture, but that only goes for the first byte. If you take
the address of the second one you're not in alignment anymore.

So what the patch does is to copy a value byte by byte into a temporary
variable the compiler has made sure is correctly aligned, do the 16/32 bit
operation on that, then copy it back. For that to work it has to operate on
uint8_t.

Re: [PATCH] usbip: Remove unaligned pointer usage from usbip tools

[shuah]

On 1/15/20 1:52 AM, Vadim Troshchinskiy wrote:
> 
> 
> ----- Mensaje original -----
>> De: "shuah" <shuah@kernel.org>
>> Para: "Vadim Troshchinskiy" <vtroshchinskiy@qindel.com>, linux-usb@vger.kernel.org
>> CC: "Valentina Manea" <valentina.manea.m@gmail.com>, "shuah" <shuah@kernel.org>
>> Enviados: Jueves, 2 de Enero 2020 1:01:24
>> Asunto: Re: [PATCH] usbip: Remove unaligned pointer usage from usbip tools
>>
>> Hi Vadim,
>>
> 
> 
> Hello! Sorry for the late reply, I was on vacation.
> 
> 

No worries. I fixed it with a simpler approach.

thanks,
-- Shuah