From: David Gwynne Subject: Re: uncm(4): process all notifications in one interrupt transfer To: Mikolaj Kucharski , tech@openbsd.org, Jan Schreiber Date: Tue, 13 Oct 2026 13:27:54 +1000 On Mon, Oct 12, 2026 at 12:18:28PM +0200, Stefan Sperling wrote: > 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 struct usb_cdc_notification is only defined in terms of uByte, uWord, and uDWord members (which is is), and only accessed with UGETW and UGETDW (which i think it is) then it's already safe to access on at any offset on big endian and/or strict alignment archs. see the top of src/sys/dev/usb/usb.h.