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

Hi all.

CDC 1.2 defines notifications as an 8-byte header carrying the length of
the data that follows.  I don't know how notifications map to transfers.
uncm_intr() assumes exactly one per transfer and silently ignores
anything after it.  This walks the buffer by wLength, with every read
bounds-checked against the received length.


My Pixel 7a still works with below diff.

# ifconfig uncm0
uncm0: flags=a48847<UP,BROADCAST,DEBUG,RUNNING,SIMPLEX,MULTICAST,AUTOCONF6TEMP,AUTOCONF6,AUTOCONF4> mtu 1500
        lladdr aa:6f:e5:6d:b6:ce
        index 6 priority 0 llprio 3
        groups: egress
        status: active
        inet6 fe80::a86f:e5ff:fe6d:b6ce%uncm0 prefixlen 64 scopeid 0x6
        inet 10.112.191.239 netmask 0xffffff00 broadcast 10.112.191.255


Since rev 1.3 the interrupt transfer is limited to wMaxPacketSize, so a
full-sized CONNECTION_SPEED_CHANGE can no longer be merged with the
following NETWORK_CONNECTION across packets.  A device whose interrupt
endpoint has a wMaxPacketSize larger than one notification may still put
several notifications into one transfer, and only the one at offset 0
was parsed.  Use each notification's wLength to advance to the next one.
Skip notifications with an unknown bNotification, but stop parsing on a
bad bmRequestType or a truncated notification, since framing can no
longer be trusted.

Index: sys/dev/usb/if_uncm.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_uncm.c,v
diff -u -p -r1.3 if_uncm.c
--- sys/dev/usb/if_uncm.c	11 Oct 2026 13:42:42 -0000	1.3
+++ sys/dev/usb/if_uncm.c	12 Oct 2026 08:40:00 -0000
@@ -1174,8 +1174,10 @@ uncm_intr(struct usbd_xfer *xfer, void *
 {
 	struct uncm_softc *sc = priv;
 	struct ifnet *ifp = GET_IFP(sc);
+	struct usb_cdc_notification *msg;
 	uint32_t up, down;
-	int	total_len;
+	uint8_t *buf;
+	int	total_len, off, dlen;
 
 	if (usbd_is_dying(sc->sc_udev))
 		return;
@@ -1198,35 +1200,60 @@ uncm_intr(struct usbd_xfer *xfer, void *
 		return;
 	}
 
-	if (sc->sc_intr_msg.bmRequestType != UCDC_NOTIFICATION) {
-		DPRINTF("%s: unexpected notification (type=0x%02x)\n",
-		    DEVNAM(sc), sc->sc_intr_msg.bmRequestType);
-		return;
-	}
+	DPRINTFN(4, "%s: recv %d bytes of notifications\n", DEVNAM(sc),
+	    total_len);
+
+	/*
+	 * A transfer may contain more than one notification: a packet
+	 * of exactly wMaxPacketSize does not end the transfer, and a
+	 * device with a large wMaxPacketSize may also pack several
+	 * notifications into one packet.  Use each notification's
+	 * wLength to find the next one.
+	 */
+	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);
+
+		if (off + UCDC_NOTIFICATION_LENGTH + dlen > total_len) {
+			DPRINTF("%s: truncated notification at offset %d\n",
+			    DEVNAM(sc), off);
+			break;
+		}
+
+		/* Framing is lost; do not parse the rest. */
+		if (msg->bmRequestType != UCDC_NOTIFICATION) {
+			DPRINTF("%s: unexpected notification (type=0x%02x)\n",
+			    DEVNAM(sc), msg->bmRequestType);
+			break;
+		}
 
-	switch (sc->sc_intr_msg.bNotification) {
-	case UCDC_N_NETWORK_CONNECTION:
-		if (ifp->if_flags & IFF_DEBUG)
-			log(LOG_DEBUG, "%s: network %sconnected\n", DEVNAM(sc),
-			    UGETW(sc->sc_intr_msg.wValue) ? "" : "dis");
-		sc->sc_link = UGETW(sc->sc_intr_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 (total_len >= UCDC_NOTIFICATION_LENGTH + 8) {
-			down = UGETDW(&sc->sc_intr_msg.data[0]);
-			up = UGETDW(&sc->sc_intr_msg.data[4]);
-			sc->sc_baudrate = MIN(up, down);
-			DPRINTFN(2, "%s: speed down %u up %u\n", DEVNAM(sc),
-			    down, up);
+		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) ?
+			    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]);
+				sc->sc_baudrate = MIN(up, down);
+				DPRINTFN(2, "%s: speed down %u up %u\n",
+				    DEVNAM(sc), down, up);
+				usb_add_task(sc->sc_udev, &sc->sc_link_task);
+			}
+			break;
+		default:
+			DPRINTF("%s: unexpected notification (0x%02x)\n",
+			    DEVNAM(sc), msg->bNotification);
+			break;
 		}
-		break;
-	default:
-		DPRINTF("%s: unexpected notification (0x%02x)\n",
-		    DEVNAM(sc), sc->sc_intr_msg.bNotification);
-		break;
 	}
 }
 

-- 
Regards,
 Mikolaj