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.
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);
smime.p7s
Description: S/MIME cryptographic signature
