RE: [PATCH] can: ucan: fix alignment constraints

[David Laight <David.Laight@ACULAB.COM>]

From: Marc Kleine-Budde
> Sent: 09 February 2021 11:28
> 
> On 09.02.2021 10:34:42, David Laight wrote:
...
> > AFAICT there is one structure that would have end-padding.
> > But I didn't actually spot anything validating it's length.
> > Which may well mean that it is possible to read off the end
> > of the USB receive buffer - plausibly into unmapped addresses.
> 
> In ucan_read_bulk_callback() there is a check of the urb's length,
> as well as the length information inside the rx'ed data:
> 
> https://elixir.bootlin.com/linux/v5.10.14/source/drivers/net/can/usb/ucan.c#L734
> 
> | static void ucan_read_bulk_callback(struct urb *urb)
> | [...]
> | 		/* check sanity (length of header) */
> | 		if ((urb->actual_length - pos) < UCAN_IN_HDR_SIZE) {
> | 			netdev_warn(up->netdev,
> | 				    "invalid message (short; no hdr; l:%d)\n",
> | 				    urb->actual_length);
> | 			goto resubmit;
> | 		}
> |
> | 		/* setup the message address */
> | 		m = (struct ucan_message_in *)
> | 			((u8 *)urb->transfer_buffer + pos);
> | 		len = le16_to_cpu(m->len);
> |
> | 		/* check sanity (length of content) */
> | 		if (urb->actual_length - pos < len) {
> | 			netdev_warn(up->netdev,
> | 				    "invalid message (short; no data; l:%d)\n",
> | 				    urb->actual_length);
> | 			print_hex_dump(KERN_WARNING,
> | 				       "raw data: ",
> | 				       DUMP_PREFIX_ADDRESS,
> | 				       16,
> | 				       1,
> | 				       urb->transfer_buffer,
> | 				       urb->actual_length,
> | 				       true);
> |
> | 			goto resubmit;
> | 		}

That looks as though it checks that the buffer length provided
by the device is all contained within the buffer.

I was looking for the check that the structure type the data
buffer is cast to fits is the supplied data.
I didn't see a 'sizeof (buffer_type)' for the one I looked for
(the structure with all the version info in it).

> 
> 
> > I looked because all the patches to 'fix' the compiler warning
> > are dubious.
> > Once you've changed the outer alignment (eg of a union) then
> > the compiler will assume that any pointer to that union is aligned.
> > So any _packed() attributes that are required for mis-aligned
> > accesses get ignored - because the compiler knows the pointer
> > must be aligned.
> 
> Here the packed attribute is not to tell the compiler, that a pointer
> to the struct ucan_message_in may be unaligned. Rather is tells the
> compiler to not add any holes into the struct.

But that isn't what it means.
Using it that way is basically wrong.
It tells the compiler that the structure might be misaligned
and, if necessary, do byte accesses and shifts.

I suggest you look at the generated code for an architecture
that doesn't have efficient unaligned accesses - eg sparc or ppc
(and probably arm32 and many others).

The compiler won't add 'random' holes.
If you want to verify that there aren't actually any holes
then use a compile-time assert on the size.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)