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@xxxxxxxxxx>
---
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
