On Mon, 2025-05-26 at 15:46 +0200, Claudio Jeker wrote: > On Mon, May 26, 2025 at 01:02:20PM +0000, Gerhard Roth wrote: > > Hi Pierre, > > > > thanks for your input. Your findings are correct. We shouldn't > > trust the values provided by a black-box USB device. > > > > > > On Fri, 2025-05-23 at 18:49 +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=284904 > > > > > > The bug report mentions: > > > > > > > in_len2mask(mask, len) will write as many as len/8 bytes: > > > > > > > > for (i = 0; i < len / 8; i++) > > > > p[i] = 0xff; > > > > > > > > len comes from a ipv4elem.prefixlen in a MBIM_CID_IP_CONFIGURATION > > > > message from the USB device, and can be any uint32_t value. So a broken > > > > or malicious USB device can cause a buffer overflow. > > > > > > I think that in reality, len comes from the network, which would make the > > > issue marginally worse. > > > > > > Can you confirm the bug on your side, and would you have any suggestion > > > as to how to fix it properly? > > > > > > I would suggest to make len an unsigned value in umb_decode_cid() and > > > subsequent calls (infolen is unsigned in the first place there) but more > > > importantly, to verify that prefixlen is at most 32. > > > > > > Does that make sense? > > > > > > HTH, > > > -- > > > khorben > > > > I would propose to limit 'prefixlen' to 32. > > > > 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.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 11:37:15 -0000 > > @@ -1838,7 +1838,8 @@ umb_add_inet_config(struct umb_softc *sc > > sin = &ifra.ifra_mask; > > sin->sin_family = AF_INET; > > sin->sin_len = sizeof (*sin); > > - in_len2mask(&sin->sin_addr, prefixlen); > > + in_len2mask(&sin->sin_addr, > > + MIN(prefixlen, sizeof (struct in_addr) * NBBY)); > > > > rv = in_ioctl(SIOCAIFADDR, (caddr_t)&ifra, ifp, 1); > > if (rv != 0) { > > > > > > > > Please do not use NBBY and instead use 8 or just hardcode 32.
Fine, updated patch below.
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 14:25:34 -0000
@@ -1838,7 +1838,8 @@ umb_add_inet_config(struct umb_softc *sc
sin = &ifra.ifra_mask;
sin->sin_family = AF_INET;
sin->sin_len = sizeof (*sin);
- in_len2mask(&sin->sin_addr, prefixlen);
+ in_len2mask(&sin->sin_addr,
+ MIN(prefixlen, sizeof (struct in_addr) * 8));
rv = in_ioctl(SIOCAIFADDR, (caddr_t)&ifra, ifp, 1);
if (rv != 0) {
smime.p7s
Description: S/MIME cryptographic signature
