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@xxxxxxxx> > > > > > > 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@xxxxxxxx> > > > > 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 |
Attachment:
signature.asc
Description: PGP signature
