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

> 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);

 		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]);
 				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;
 		}
 	}