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

On 09.02.2021 10:34:42, David Laight wrote:

From: Marc Kleine-Budde

quoted

Sent: 08 February 2021 13:16

On 04.02.2021 17:26:13, Arnd Bergmann wrote:

quoted

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]

quoted

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 |