From: Mikolaj Kucharski <mikolaj@kucharski.name>
Subject: Re: uncm(4): process all notifications in one interrupt transfer
To: Jan Schreiber <jes@posteo.de>
Cc: tech@openbsd.org, stsp@stsp.name
Date: Mon, 12 Oct 2026 17:36:17 +0000

Jan.

Thank you looking into this. See my comments below.


On Mon, Oct 12, 2026 at 01:43:12PM +0000, Jan Schreiber wrote:
> > Could we copy these messages out of sc_intr_msg into another variable
> > of struct usb_cdc_notification type with memcpy() after checking sizes?
> >
> > We must avoid unaligned access on some platforms OpenBSD supports. Just
> > casting a uint8_t * type to a struct * type could make us end up in ddb
> > on such platforms if the device doesn't align its messages as expected
> > by the platform.
> >
> > I realize this makes the driver slower and we're already copying data from
> > and to USB transfers. But complicated message framing like this has a cost.
> 
> Reading the struct definition it looks to me like it's currently safe on
> every supported OpenBSD platform. I do agree with your statement though
> and this should protect us from future problems.
> 
> Tested with my Pixel 9 Pro and still reaching > 1 Gbits/sec
> 
> Diff to apply on top of Mikolajs patch.
> 
> diff --git sys/dev/usb/if_uncm.c sys/dev/usb/if_uncm.c
> index a003841a7b7..1126d29870d 100644
> --- sys/dev/usb/if_uncm.c
> +++ sys/dev/usb/if_uncm.c
> @@ -1174,7 +1174,7 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  {
>  	struct uncm_softc *sc = priv;
>  	struct ifnet *ifp = GET_IFP(sc);
> -	struct usb_cdc_notification *msg;
> +	struct usb_cdc_notification msg;
>  	uint32_t up, down;
>  	uint8_t *buf;
>  	int	total_len, off, dlen;
> @@ -1213,8 +1213,8 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  	buf = (uint8_t *)&sc->sc_intr_msg;
>  	for (off = 0; off + UCDC_NOTIFICATION_LENGTH <= total_len;
>  	    off += UCDC_NOTIFICATION_LENGTH + dlen) {
> -		msg = (struct usb_cdc_notification *)(buf + off);
> -		dlen = UGETW(msg->wLength);
> +		memcpy(&msg, buf + off, UCDC_NOTIFICATION_LENGTH);
> +		dlen = UGETW(msg.wLength);

from sys/dev/usb/usbcdc.h I see that:

	#define UCDC_NOTIFICATION_LENGTH 8

So memcpy() copies 8 bytes.


>  		if (off + UCDC_NOTIFICATION_LENGTH + dlen > total_len) {
>  			DPRINTF("%s: truncated notification at offset %d\n",
> @@ -1223,26 +1223,26 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  		}
> 
>  		/* Framing is lost; do not parse the rest. */
> -		if (msg->bmRequestType != UCDC_NOTIFICATION) {
> +		if (msg.bmRequestType != UCDC_NOTIFICATION) {
>  			DPRINTF("%s: unexpected notification (type=0x%02x)\n",
> -			    DEVNAM(sc), msg->bmRequestType);
> +			    DEVNAM(sc), msg.bmRequestType);
>  			break;
>  		}
> 
> -		switch (msg->bNotification) {
> +		switch (msg.bNotification) {
>  		case UCDC_N_NETWORK_CONNECTION:
>  			if (ifp->if_flags & IFF_DEBUG)
>  				log(LOG_DEBUG, "%s: network %sconnected\n",
>  				    DEVNAM(sc),
> -				    UGETW(msg->wValue) ? "" : "dis");
> -			sc->sc_link = UGETW(msg->wValue) ?
> +				    UGETW(msg.wValue) ? "" : "dis");
> +			sc->sc_link = UGETW(msg.wValue) ?
>  			    LINK_STATE_UP : LINK_STATE_DOWN;
>  			usb_add_task(sc->sc_udev, &sc->sc_link_task);
>  			break;
>  		case UCDC_N_CONNECTION_SPEED_CHANGE:
>  			if (dlen >= 8) {
> -				down = UGETDW(&msg->data[0]);
> -				up = UGETDW(&msg->data[4]);
> +				down = UGETDW(&msg.data[0]);
> +				up = UGETDW(&msg.data[4]);

I think memcpy() didn't copy `uByte data[16]` into msg struct.
As far as I understand, accessing &msg->data[] is not correct
in this case.


>  				sc->sc_baudrate = MIN(up, down);
>  				DPRINTFN(2, "%s: speed down %u up %u\n",
>  				    DEVNAM(sc), down, up);
> @@ -1251,7 +1251,7 @@ uncm_intr(struct usbd_xfer *xfer, void *priv, usbd_status status)
>  			break;
>  		default:
>  			DPRINTF("%s: unexpected notification (0x%02x)\n",
> -			    DEVNAM(sc), msg->bNotification);
> +			    DEVNAM(sc), msg.bNotification);
>  			break;
>  		}
>  	}
> 

-- 
Regards,
 Mikolaj