On Mon, May 26, 2025 at 01:04:09PM +0000, Gerhard Roth wrote:
> Hi Pierre,
>
>
> On Sun, 2025-05-25 at 12:28 +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=284906
> >
> > The bug report mentions:
> >
> > > if_umb.c calls umb_getinfobuf() with offs and size taken from messages
> > > sent by the USB device. The "inlen >= offs + sz" check isn't
> > > sufficient due to possible integer wrap. This can allow a broken or
> > > malicious USB device to cause a buffer overflow.
> >
> > From my reading of the current version of the umb(4) driver in OpenBSD,
> > ISTM that you are vulnerable to this issue as well.
> >
> > At the very least I would suggest to keep inlen and len unsigned
> > throughout, and then I think there should be a check for integer overflow
> > in umb_getinfobuf() as suggested.
> >
> > Thoughts?
> >
> > HTH,
>
> converting the variables to 64bit integer should help to prevent
> any overflows.
>
> And since the function is void, we should zeroize the returned
> data struct at all times.
OK 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:31:09 -0000
> @@ -1433,10 +1433,9 @@ umb_getinfobuf(void *in, int inlen, uint
> {
> offs = letoh32(offs);
> sz = letoh32(sz);
> - if (inlen >= offs + sz) {
> - memset(out, 0, outlen);
> + memset(out, 0, outlen);
> + if ((uint64_t)inlen >= (uint64_t)offs + (uint64_t)sz)
> memcpy(out, in + offs, MIN(sz, outlen));
> - }
> }
>
> static inline int
>
>
