On Tue, Oct 13, 2026 at 01:27:54PM +1000, David Gwynne wrote:
> 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<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 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.

In that case I'm happy to OK (for what its worth) Mikolajs original
diff. Thank you for pointing it out, David!