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()?
And are your casts necessary? The compiler should handle that
himself now.
Anyway, better than before, OK bluhm@
> 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;
>
>
