Hi Philippe,
On Mon, Feb 13, 2023 at 7:26 PM Philippe Mathieu-Daudé
<phi...@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 <ningqia...@huawei.com>
Reported-by: SorryMybad of Kunlun Lab <soulchen8...@gmail.com>
> > Signed-off-by: Mauro Matteo Cascella <mcasc...@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
