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. -- :wq Claudio
