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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3927 bytes --]

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]