Re: [PATCH] usb/dev-wacom: fix OOB write in usb_mouse_poll()

[Mauro Matteo Cascella]

Hi Philippe,

On Mon, Feb 13, 2023 at 7:26 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Mauro,
>
> On 13/2/23 18:41, Mauro Matteo Cascella wrote:
> > The guest can control the size of buf; an OOB write occurs when buf is 1 or 
> > 2
> > bytes long. Only fill in the buffer as long as there is enough space, throw
> > away any data which doesn't fit.
>
> Any reproducer?

No qtest reproducer, we do have a PoC module to compile & load from
within the guest. This is ASAN output:

=================================================================
==28803==ERROR: AddressSanitizer: heap-buffer-overflow on address 0
WRITE of size 1 at 0x602000fccdd1 thread T2
    #0 0x560f4ebba899 in usb_mouse_poll ../hw/usb/dev-wacom.c:256
    #1 0x560f4ebbcaf9 in usb_wacom_handle_data ../hw/usb/dev-wacom6
    #2 0x560f4eaef297 in usb_device_handle_data ../hw/usb/bus.c:180
    #3 0x560f4eb00bbb in usb_process_one ../hw/usb/core.c:406
    #4 0x560f4eb01883 in usb_handle_packet ../hw/usb/core.c:438
    #5 0x560f4eb94e0c in xhci_submit ../hw/usb/hcd-xhci.c:1801
    #6 0x560f4eb9505f in xhci_fire_transfer ../hw/usb/hcd-xhci.c:10
    #7 0x560f4eb9773c in xhci_kick_epctx ../hw/usb/hcd-xhci.c:1969
    #8 0x560f4eb953f2 in xhci_kick_ep ../hw/usb/hcd-xhci.c:1835
    #9 0x560f4eba416d in xhci_doorbell_write ../hw/usb/hcd-xhci.c:7
    #10 0x560f4f5343a8 in memory_region_write_accessor ../softmmu/2
    #11 0x560f4f53483f in access_with_adjusted_size ../softmmu/mem4
    #12 0x560f4f541e69 in memory_region_dispatch_write ../softmmu/4
    #13 0x560f4f57afec in flatview_write_continue ../softmmu/physm5
    #14 0x560f4f57b40f in flatview_write ../softmmu/physmem.c:2867
    #15 0x560f4f579617 in subpage_write ../softmmu/physmem.c:2501
    #16 0x560f4f5346dc in memory_region_write_with_attrs_accessor 3
    #17 0x560f4f53483f in access_with_adjusted_size ../softmmu/mem4
    #18 0x560f4f542075 in memory_region_dispatch_write ../softmmu/1
    #19 0x560f4f727735 in io_writex ../accel/tcg/cputlb.c:1429
    #20 0x560f4f72c19d in store_helper ../accel/tcg/cputlb.c:2379
    #21 0x560f4f72c5ec in full_le_stl_mmu ../accel/tcg/cputlb.c:247
    #22 0x560f4f72c62a in helper_le_stl_mmu ../accel/tcg/cputlb.c:3
    #23 0x7fcf063941a3  (/memfd:tcg-jit (deleted)+0x27541a3)
    <cut>

Also forgot to give credits:

Reported-by: ningqiang1 <ningqiang1@huawei.com>
Reported-by: SorryMybad of Kunlun Lab <soulchen8650@gmail.com>

> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > ---
> >   hw/usb/dev-wacom.c | 20 +++++++++++++-------
> >   1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
> > index 7177c17f03..ca9e6aa82f 100644
> > --- a/hw/usb/dev-wacom.c
> > +++ b/hw/usb/dev-wacom.c
> > @@ -252,14 +252,20 @@ static int usb_mouse_poll(USBWacomState *s, uint8_t 
> > *buf, int len)
> >       if (s->buttons_state & MOUSE_EVENT_MBUTTON)
> >           b |= 0x04;
> >
> > -    buf[0] = b;
> > -    buf[1] = dx;
> > -    buf[2] = dy;
> > -    l = 3;
> > -    if (len >= 4) {
> > -        buf[3] = dz;
> > -        l = 4;
> > +    l = 0;
> > +    if (len > l) {
> > +        buf[l++] = b;
> >       }
> > +    if (len > l) {
> > +        buf[l++] = dx;
> > +    }
>
>         else { // the packet is now corrupted... }
>
> > +    if (len > l) {
> > +        buf[l++] = dy;
> > +    }
> > +    if (len > l) {
> > +        buf[l++] = dz;
> > +    }
> > +
> >       return l;
> >   }
>
> Better is to wait for enough data to process:
>
> -- >8 --
> diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
> index 7177c17f03..2fe2a9220e 100644
> --- a/hw/usb/dev-wacom.c
> +++ b/hw/usb/dev-wacom.c
> @@ -244,6 +244,9 @@ static int usb_mouse_poll(USBWacomState *s, uint8_t
> *buf, int len)
>       s->dy -= dy;
>       s->dz -= dz;
>
> +    if (len < 3)
> +        return 0;
> +
>       b = 0;
>       if (s->buttons_state & MOUSE_EVENT_LBUTTON)
>           b |= 0x01;
> @@ -274,6 +277,9 @@ static int usb_wacom_poll(USBWacomState *s, uint8_t
> *buf, int len)
>           s->mouse_grabbed = 1;
>       }
>
> +    if (len < 7)
> +        return 0;
> +
>       b = 0;
>       if (s->buttons_state & MOUSE_EVENT_LBUTTON)
>           b |= 0x01;
> @@ -282,9 +288,6 @@ static int usb_wacom_poll(USBWacomState *s, uint8_t
> *buf, int len)
>       if (s->buttons_state & MOUSE_EVENT_MBUTTON)
>           b |= 0x20; /* eraser */
>
> -    if (len < 7)
> -        return 0;
> -
>       buf[0] = s->mode;
>       buf[5] = 0x00 | (b & 0xf0);
>       buf[1] = s->x & 0xff;
> ---
>

I took inspiration from hid_pointer_poll() in hw/input/hid.c which
fills in the buffer as much as possible in a similar way, but your
suggestion makes sense to me. Gerd, wdyt?

Thanks,
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0