lkml.org 
[lkml]   [2021]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] can: ucan: fix alignment constraints
On 09.02.2021 10:34:42, David Laight wrote:
> From: Marc Kleine-Budde
> > Sent: 08 February 2021 13:16
> >
> > On 04.02.2021 17:26:13, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > struct ucan_message_in contains member with 4-byte alignment
> > > but is itself marked as unaligned, which triggers a warning:
> > >
> > > drivers/net/can/usb/ucan.c:249:1: warning: alignment 1 of 'struct ucan_message_in' is less than 4 [-
> > Wpacked-not-aligned]
> > >
> > > Mark the outer structure to have the same alignment as the inner
> > > one.
> > >
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Applied to linux-can-next/testing.
>
> I've just had a look at that file.
>
> Are any of the __packed or __aligned actually required at all.
>
> 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;
| }


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

| struct ucan_message_in {
| __le16 len; /* Length of the content include header */
| u8 type; /* UCAN_IN_RX and friends */
| u8 subtype; /* command sub type */
|
| union {
| /* CAN Frame received
| * (type == UCAN_IN_RX)
| * && ((msg.can_msg.id & CAN_RTR_FLAG) == 0)
| */
| struct ucan_can_msg can_msg;
|
| /* CAN transmission complete
| * (type == UCAN_IN_TX_COMPLETE)
| */
| struct ucan_tx_complete_entry_t can_tx_complete_msg[0];
| } __aligned(0x4) msg;
| } __packed;

> So while the changes remove the warning, they may be removing
> support for misaligned addresses.

There won't be any misaligned access on the struct, the USB device
will send it aligned and the driver will enforce the alignment:

| /* proceed to next message */
| pos += len;
| /* align to 4 byte boundary */
| pos = round_up(pos, 4);

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-02-09 12:47    [W:0.099 / U:0.588 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site