From: Stefan Sperling Subject: Re: uncm(4): process all notifications in one interrupt transfer To: Mikolaj Kucharski Cc: tech@openbsd.org, Jan Schreiber Date: Mon, 12 Oct 2026 12:18:28 +0200 On Mon, Oct 12, 2026 at 09:55:55AM +0000, Mikolaj Kucharski wrote: > 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 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); 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. > + 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 >