On Mon, 2025-05-26 at 16:38 +0200, [email protected] wrote: > On Mon, May 26, 2025 at 02:26:55PM +0000, Gerhard Roth wrote: > > 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. > > Maybe it would be better to fail hard in that case and return an error.
in6_prefixlen2mask() is even uglier! It logs an error and returns even
before bzero'ing the returned mask.
>
> Diff below is OK claudio@
>
> > 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
