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

[Philippe Mathieu-Daudé]

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?

> 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;
---