Download raw body.
uncm(4): process all notifications in one interrupt transfer
uncm(4): process all notifications in one interrupt transfer
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<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);
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
>
uncm(4): process all notifications in one interrupt transfer
uncm(4): process all notifications in one interrupt transfer