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
> Index: sys/dev/usb/if_umb.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> diff -u -p -u -p -r1.59 if_umb.c
> --- sys/dev/usb/if_umb.c 8 Aug 2024 05:10:00 -0000 1.59
> +++ sys/dev/usb/if_umb.c 26 May 2025 12:53:44 -0000
> @@ -2458,10 +2458,12 @@ umb_decap(struct umb_softc *sc, struct u
> 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 + (uint64_t)ptroff)
> goto toosmall;
> if (!MBIM_NCM_NTH16_ISISG(psig) && !MBIM_NCM_NTH32_ISISG(psig)) {
> DPRINTF("%s: unsupported NCM pointer signature (0x%08x)\n",
> @@ -2508,7 +2510,7 @@ 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 < (uint64_t)dlen + (uint64_t)doff) {
> /* Skip giant datagram but continue processing */
> DPRINTF("%s: datagram too large (%d @ off %d)\n",
> DEVNAM(sc), dlen, doff);
>
