| Message ID | 20210204162625.3099392-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | can: ucan: fix alignment constraints | expand |
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. Thanks, 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 |
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. 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. So while the changes remove the warning, they may be removing support for misaligned addresses. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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 |
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)
diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c index fa403c080871..536c985dad21 100644 --- a/drivers/net/can/usb/ucan.c +++ b/drivers/net/can/usb/ucan.c @@ -246,7 +246,7 @@ struct ucan_message_in { */ struct ucan_tx_complete_entry_t can_tx_complete_msg[0]; } __aligned(0x4) msg; -} __packed; +} __packed __aligned(0x4); /* Macros to calculate message lengths */ #define UCAN_OUT_HDR_SIZE offsetof(struct ucan_message_out, msg)