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

[Vadim Troshchinskiy <vtroshchinskiy@qindel.com>]

----- 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.