From: shuah <shuah@kernel.org>
To: Vadim Troshchinskiy <vtroshchinskiy@qindel.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Cc: Valentina Manea <valentina.manea.m@gmail.com>, shuah <shuah@kernel.org>
Subject: Re: [PATCH] usbip: Remove unaligned pointer usage from usbip tools
Date: Wed, 1 Jan 2020 17:01:24 -0700 [thread overview]
Message-ID: <86e6dfbf-cf51-1467-3a78-fd72377385b7@kernel.org> (raw)
In-Reply-To: <5176009.64u6Zm7RkX@gverdu.qindel.com>
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
next prev parent reply other threads:[~2020-01-02 0:01 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-10 15:50 [PATCH] usbip: Remove unaligned pointer usage from usbip tools Vadim Troshchinskiy
2020-01-02 0:01 ` shuah [this message]
2020-01-15 8:52 ` Vadim Troshchinskiy
2020-01-17 16:09 ` shuah
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86e6dfbf-cf51-1467-3a78-fd72377385b7@kernel.org \
--to=shuah@kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=valentina.manea.m@gmail.com \
--cc=vtroshchinskiy@qindel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).