On Fri, 2025-06-06 at 16:39 +0200, Alexander Bluhm wrote: > > On Thu, Jun 05, 2025 at 11:58:26AM +0000, Gerhard Roth wrote: > > > > On Wed, 2025-06-04 at 19:11 +0200, Alexander Bluhm wrote: > > > > > > On Mon, May 26, 2025 at 01:05:51PM +0000, Gerhard Roth wrote: > > > > > > > > Hi Pierre, > > > > > > > > > > > > > > > > On Sun, 2025-05-25 at 12:44 +0200, Pierre Pronchery wrote: > > > > > > > > > > ????????????????????????????????????????????????Dear > > > > > > > > > > OpenBSD team, > > > > > > > > > > > > > > > > > > > > I would like to bring your attention to the following bug > > > > > > > > > > report from > > > > > > > > > > FreeBSD, where I have ported and imported the umb(4) driver > > > > > > > > > > recently: > > > > > > > > > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=284920 > > > > > > > > > > > > > > > > > > > > The bug report mentions: > > > > > > > > > > > > > > > > > > > > > > When processing a message produced by a USB device, > > > > > > > > > > > > umb_decap() > > > > > > > > > > > > says: > > > > > > > > > > > > > > > > > > > > > > > > ptroff = UGETDW(hdr32->dwNdpIndex); > > > > > > > > > > > > ...; > > > > > > > > > > > > ptr16 = (struct ncm_pointer16 *)(buf + ptroff); > > > > > > > > > > > > psig = UGETDW(ptr16->dwSignature); > > > > > > > > > > > > > > > > > > > > > > > > But ptroff can be any 32-bit value, so the a broken or > > > > > > > > > > > > malicious USB > > > > > > > > > > > > device can cause ptr16 to point outside the message > > > > > > > > > > > > buffer. > > > > > > > > > > > > > > > > > > > > And: > > > > > > > > > > > > > > > > > > > > > > And later, umb_decap() pulls dlen and doff out of a > > > > > > > > > > > > message sent by > > > > > > > > > > > > the USB device, and uses doff to form a pointer without > > > > > > > > > > > > a sanity > > > > > > > > > > > > check: > > > > > > > > > > > > > > > > > > > > > > > > dgram32 = (struct ncm_pointer32_dgram *) > > > > > > > > > > > > (buf + ptroff + dgentryoff); > > > > > > > > > > > > ...; > > > > > > > > > > > > dlen = UGETDW(dgram32->dwDatagramLen); > > > > > > > > > > > > doff = UGETDW(dgram32->dwDatagramIndex); > > > > > > > > > > > > ...; > > > > > > > > > > > > dp = buf + doff; > > > > > > > > > > > > ...; > > > > > > > > > > > > m = m_devget(dp, dlen, 0, ifp, NULL); > > > > > > > > > > > > > > > > > > > > > > > > A malicious USB device could cause the wrong memory to > > > > > > > > > > > > be copied, or a > > > > > > > > > > > > page fault. > > > > > > > > > > > > > > > > > > > > From my reading of the current version of the umb(4) driver > > > > > > > > > > in OpenBSD, > > > > > > > > > > ISTM that you are vulnerable to this issue as well. > > > > > > > > > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > > > > > > > > > HTH, > > > > > > > > > > > > > > > > 64bit integers should prevent overlows in the check. > > > > > > > > Also validate 'ptroff' before using it the first time. > > > > > > > > > > > > I would prefer to have uint32_t variables for values coming from > > > > > > UGETW() and uint64_t for UGETDW().?? Then I don't have to think > > > > > > about > > > > > > negative values and sign conversions in all the other comparions. > > > > > > > > > > > > uint32_t ptrlen, dgentryoff; > > > > > > uint64_t blen, ptroff, dlen, doff; > > > > > > > > > > > > bluhm > > > > > > > > > > > > > > Thanks for the review, updated diff below. > > > > Can (int)dlen be negative? I am not an USB expert, but it seems > > that this length ultimatly comes from sc_rx_bufsz or sc_tx_bufsz. > > There I see another UGETDW(np.dwNtbInMaxSize) and > > UGETDW(np.dwNtbOutMaxSize). Do we need a size check in umb_ncm_setup()?
You probably mean 'len' (instead of 'dlen').
I agree that an additional check in umb_ncm_setup() would be
a good idea.
Please look at the proposed patch at the end of this mail.
> >
> > And are your casts necessary? The compiler should handle that
> > himself now.
Maybe. But C integer promotions can be weird sometimes.
I'd prefer keeping the casts.
> >
> > Anyway, better than before, OK bluhm@
Thanks for the review.
Gerhard
> >
> > > > Index: sys/dev/usb/if_umb.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> > > > diff -u -p -u -p -r1.61 if_umb.c
> > > > --- sys/dev/usb/if_umb.c 5 Jun 2025 08:22:25 -0000 1.61
> > > > +++ sys/dev/usb/if_umb.c 5 Jun 2025 11:54:00 -0000
> > > > @@ -2407,9 +2407,8 @@ umb_decap(struct umb_softc *sc, struct u
> > > > struct ncm_pointer16_dgram *dgram16;
> > > > struct ncm_pointer32_dgram *dgram32;
> > > > uint32_t hsig, psig;
> > > > - int blen;
> > > > - int ptrlen, ptroff, dgentryoff;
> > > > - uint32_t doff, dlen;
> > > > + uint32_t ptrlen, dgentryoff;
> > > > + uint64_t blen, ptroff, doff, dlen;
> > > > struct mbuf_list ml = MBUF_LIST_INITIALIZER();
> > > > struct mbuf *m;
> > > >
> > > > @@ -2453,15 +2452,17 @@ umb_decap(struct umb_softc *sc, struct u
> > > > goto fail;
> > > > }
> > > > if (blen != 0 && len < blen) {
> > > > - DPRINTF("%s: bad NTB len (%d) for %d bytes of data\n",
> > > > + DPRINTF("%s: bad NTB len (%llu) for %d bytes of data\n",
> > > > DEVNAM(sc), blen, len);
> > > > goto fail;
> > > > }
> > > >
> > > > + if (len < ptroff)
> > > > + goto toosmall;
> > > > ptr16 = (struct ncm_pointer16 *)(buf + ptroff);
> > > > psig = UGETDW(ptr16->dwSignature);
> > > > ptrlen = UGETW(ptr16->wLength);
> > > > - if (len < ptrlen + ptroff)
> > > > + if ((uint64_t)len < (uint64_t)ptrlen + ptroff)
> > > > goto toosmall;
> > > > if (!MBIM_NCM_NTH16_ISISG(psig) && !MBIM_NCM_NTH32_ISISG(psig))
> > > > {
> > > > DPRINTF("%s: unsupported NCM pointer signature
> > > > (0x%08x)\n",
> > > > @@ -2508,16 +2509,16 @@ umb_decap(struct umb_softc *sc, struct u
> > > > /* Terminating zero entry */
> > > > if (dlen == 0 || doff == 0)
> > > > break;
> > > > - if (len < dlen + doff) {
> > > > + if ((uint64_t)len < dlen + doff) {
> > > > /* Skip giant datagram but continue processing
> > > > */
> > > > - DPRINTF("%s: datagram too large (%d @ off
> > > > %d)\n",
> > > > + DPRINTF("%s: datagram too large (%llu @ off
> > > > %llu)\n",
> > > > DEVNAM(sc), dlen, doff);
> > > > continue;
> > > > }
> > > >
> > > > dp = buf + doff;
> > > > - DPRINTFN(3, "%s: decap %d bytes\n", DEVNAM(sc), dlen);
> > > > - m = m_devget(dp, dlen, sizeof(uint32_t));
> > > > + DPRINTFN(3, "%s: decap %llu bytes\n", DEVNAM(sc), dlen);
> > > > + m = m_devget(dp, (int)dlen, sizeof(uint32_t));
> > > > if (m == NULL) {
> > > > ifp->if_iqdrops++;
> > > > continue;
> > > >
> > > >
> >
> >
Index: sys/dev/usb/if_umb.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
diff -u -p -u -p -r1.62 if_umb.c
--- sys/dev/usb/if_umb.c 10 Jun 2025 08:29:57 -0000 1.62
+++ sys/dev/usb/if_umb.c 10 Jun 2025 09:31:02 -0000
@@ -359,6 +359,7 @@ umb_attach(struct device *parent, struct
int altnum;
int s;
struct ifnet *ifp;
+ int maxpktlen;
sc->sc_udev = uaa->device;
sc->sc_ctrl_ifaceno = uaa->ifaceno;
@@ -461,10 +462,14 @@ umb_attach(struct device *parent, struct
MBIM_CTRLMSG_MAXLEN);
/* cont. anyway */
}
- sc->sc_maxpktlen = UGETW(md->wMaxSegmentSize);
- DPRINTFN(2, "%s: ctrl_len=%d, maxpktlen=%d, cap=0x%x\n",
- DEVNAM(sc), sc->sc_ctrl_len, sc->sc_maxpktlen,
- md->bmNetworkCapabilities);
+ maxpktlen = UGETW(md->wMaxSegmentSize);
+ if (maxpktlen > 0 && maxpktlen <= INT32_MAX) {
+ sc->sc_maxpktlen = maxpktlen;
+ DPRINTFN(2, "%s: ctrl_len=%d, maxpktlen=%d, "
+ "cap=0x%x\n", DEVNAM(sc), sc->sc_ctrl_len,
+ sc->sc_maxpktlen,
+ md->bmNetworkCapabilities);
+ }
break;
default:
break;
@@ -709,8 +714,10 @@ umb_ncm_setup(struct umb_softc *sc)
USETW(req.wLength, sizeof (np));
if (usbd_do_request(sc->sc_udev, &req, &np) == USBD_NORMAL_COMPLETION &&
UGETW(np.wLength) == sizeof (np)) {
- sc->sc_rx_bufsz = UGETDW(np.dwNtbInMaxSize);
- sc->sc_tx_bufsz = UGETDW(np.dwNtbOutMaxSize);
+ sc->sc_rx_bufsz = MIN(UGETDW(np.dwNtbInMaxSize),
+ (uint32_t)sc->sc_maxpktlen);
+ sc->sc_tx_bufsz = MIN(UGETDW(np.dwNtbOutMaxSize),
+ (uint32_t)sc->sc_maxpktlen);
sc->sc_maxdgram = UGETW(np.wNtbOutMaxDatagrams);
sc->sc_align = UGETW(np.wNdpOutAlignment);
sc->sc_ndp_div = UGETW(np.wNdpOutDivisor);
smime.p7s
Description: S/MIME cryptographic signature
