[PULL 0/6] Usb 20210505 patches

[PULL 0/6] Usb 20210505 patches

[Gerd Hoffmann]

The following changes since commit 15106f7dc3290ff3254611f265849a314a93eb0e:

  Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-hex-20210502' into staging (2021-05-02 16:23:05 +0100)

are available in the Git repository at:

  git://git.kraxel.org/qemu tags/usb-20210505-pull-request

for you to fetch changes up to 05a40b172e4d691371534828078be47e7fff524c:

  usb: limit combined packets to 1 MiB (CVE-2021-3527) (2021-05-05 15:06:01 +0200)

----------------------------------------------------------------
usb: fix some memory allocation issues (CVE-2021-3527).
usb: add stubs, allow building without usb subsystem.

----------------------------------------------------------------

Gerd Hoffmann (4):
  usb/hid: avoid dynamic stack allocation
  usb/redir: avoid dynamic stack allocation (CVE-2021-3527)
  usb/mtp: avoid dynamic stack allocation
  usb: limit combined packets to 1 MiB (CVE-2021-3527)

Philippe Mathieu-Daudé (2):
  hw/usb/host-stub: Remove unused header
  hw/usb: Do not build USB subsystem if not required

 hw/usb/combined-packet.c |  4 +++-
 hw/usb/dev-hid.c         |  2 +-
 hw/usb/dev-mtp.c         |  3 ++-
 hw/usb/dev-wacom.c       |  2 +-
 hw/usb/host-stub.c       |  1 -
 hw/usb/redirect.c        |  6 +++---
 stubs/usb-dev-stub.c     | 25 +++++++++++++++++++++++++
 MAINTAINERS              |  1 +
 hw/usb/meson.build       |  9 +++------
 stubs/meson.build        |  1 +
 10 files changed, 40 insertions(+), 14 deletions(-)
 create mode 100644 stubs/usb-dev-stub.c

-- 
2.31.1

[PULL 1/6] hw/usb/host-stub: Remove unused header

[Gerd Hoffmann]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20210424224110.3442424-2-f4bug@amsat.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-stub.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/usb/host-stub.c b/hw/usb/host-stub.c
index 538ed29684cb..80809ceba542 100644
--- a/hw/usb/host-stub.c
+++ b/hw/usb/host-stub.c
@@ -31,7 +31,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "ui/console.h"
 #include "hw/usb.h"
 #include "monitor/monitor.h"
 
-- 
2.31.1

[PULL 2/6] hw/usb: Do not build USB subsystem if not required

[Gerd Hoffmann]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

If the Kconfig 'USB' value is not selected, it is pointless to
build the USB core components. Add a stub for the HMP commands
and usbdevice_create() which is called by usb_device_add in
softmmu/vl.c.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20210424224110.3442424-3-f4bug@amsat.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 stubs/usb-dev-stub.c | 25 +++++++++++++++++++++++++
 MAINTAINERS          |  1 +
 hw/usb/meson.build   |  9 +++------
 stubs/meson.build    |  1 +
 4 files changed, 30 insertions(+), 6 deletions(-)
 create mode 100644 stubs/usb-dev-stub.c

diff --git a/stubs/usb-dev-stub.c b/stubs/usb-dev-stub.c
new file mode 100644
index 000000000000..b1adeeb4548d
--- /dev/null
+++ b/stubs/usb-dev-stub.c
@@ -0,0 +1,25 @@
+/*
+ * QEMU USB device emulation stubs
+ *
+ * Copyright (C) 2021 Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "sysemu/sysemu.h"
+#include "monitor/monitor.h"
+#include "hw/usb.h"
+
+USBDevice *usbdevice_create(const char *driver)
+{
+    error_report("Support for USB devices not built-in");
+
+    return NULL;
+}
+
+void hmp_info_usb(Monitor *mon, const QDict *qdict)
+{
+    monitor_printf(mon, "Support for USB devices not built-in\n");
+}
diff --git a/MAINTAINERS b/MAINTAINERS
index 4c05ff8bbabd..6f7e5db3b102 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1804,6 +1804,7 @@ USB
 M: Gerd Hoffmann <kraxel@redhat.com>
 S: Maintained
 F: hw/usb/*
+F: stubs/usb-dev-stub.c
 F: tests/qtest/usb-*-test.c
 F: docs/usb2.txt
 F: docs/usb-storage.txt
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index fb7a74e73ae8..f357270d0b6b 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -1,17 +1,14 @@
 hw_usb_modules = {}
 
 # usb subsystem core
-softmmu_ss.add(files(
+softmmu_ss.add(when: 'CONFIG_USB', if_true: files(
   'bus.c',
   'combined-packet.c',
   'core.c',
-  'pcap.c',
-  'libhw.c'
-))
-
-softmmu_ss.add(when: 'CONFIG_USB', if_true: files(
   'desc.c',
   'desc-msos.c',
+  'libhw.c',
+  'pcap.c',
 ))
 
 # usb host adapters
diff --git a/stubs/meson.build b/stubs/meson.build
index be6f6d609e58..3faef16892be 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -50,6 +50,7 @@ if have_block
 endif
 if have_system
   stub_ss.add(files('semihost.c'))
+  stub_ss.add(files('usb-dev-stub.c'))
   stub_ss.add(files('xen-hw-stub.c'))
 else
   stub_ss.add(files('qdev.c'))
-- 
2.31.1

[PULL 3/6] usb/hid: avoid dynamic stack allocation

[Gerd Hoffmann]

Use autofree heap allocation instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210503132915.2335822-2-kraxel@redhat.com>
---
 hw/usb/dev-hid.c   | 2 +-
 hw/usb/dev-wacom.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
index fc39bab79f94..1c7ae97c3033 100644
--- a/hw/usb/dev-hid.c
+++ b/hw/usb/dev-hid.c
@@ -656,7 +656,7 @@ static void usb_hid_handle_data(USBDevice *dev, USBPacket *p)
 {
     USBHIDState *us = USB_HID(dev);
     HIDState *hs = &us->hid;
-    uint8_t buf[p->iov.size];
+    g_autofree uint8_t *buf = g_malloc(p->iov.size);
     int len = 0;
 
     switch (p->pid) {
diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
index b59504863509..ed687bc9f1eb 100644
--- a/hw/usb/dev-wacom.c
+++ b/hw/usb/dev-wacom.c
@@ -301,7 +301,7 @@ static void usb_wacom_handle_control(USBDevice *dev, USBPacket *p,
 static void usb_wacom_handle_data(USBDevice *dev, USBPacket *p)
 {
     USBWacomState *s = (USBWacomState *) dev;
-    uint8_t buf[p->iov.size];
+    g_autofree uint8_t *buf = g_malloc(p->iov.size);
     int len = 0;
 
     switch (p->pid) {
-- 
2.31.1

[PULL 4/6] usb/redir: avoid dynamic stack allocation (CVE-2021-3527)

[Gerd Hoffmann]

Use autofree heap allocation instead.

Fixes: 4f4321c11ff ("usb: use iovecs in USBPacket")
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210503132915.2335822-3-kraxel@redhat.com>
---
 hw/usb/redirect.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 17f06f34179a..6a75b0dc4ab2 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -620,7 +620,7 @@ static void usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
                 .endpoint = ep,
                 .length = p->iov.size
             };
-            uint8_t buf[p->iov.size];
+            g_autofree uint8_t *buf = g_malloc(p->iov.size);
             /* No id, we look at the ep when receiving a status back */
             usb_packet_copy(p, buf, p->iov.size);
             usbredirparser_send_iso_packet(dev->parser, 0, &iso_packet,
@@ -818,7 +818,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
         usbredirparser_send_bulk_packet(dev->parser, p->id,
                                         &bulk_packet, NULL, 0);
     } else {
-        uint8_t buf[size];
+        g_autofree uint8_t *buf = g_malloc(size);
         usb_packet_copy(p, buf, size);
         usbredir_log_data(dev, "bulk data out:", buf, size);
         usbredirparser_send_bulk_packet(dev->parser, p->id,
@@ -923,7 +923,7 @@ static void usbredir_handle_interrupt_out_data(USBRedirDevice *dev,
                                                USBPacket *p, uint8_t ep)
 {
     struct usb_redir_interrupt_packet_header interrupt_packet;
-    uint8_t buf[p->iov.size];
+    g_autofree uint8_t *buf = g_malloc(p->iov.size);
 
     DPRINTF("interrupt-out ep %02X len %zd id %"PRIu64"\n", ep,
             p->iov.size, p->id);
-- 
2.31.1

[PULL 5/6] usb/mtp: avoid dynamic stack allocation

[Gerd Hoffmann]

Use autofree heap allocation instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210503132915.2335822-4-kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index bbb827434482..2a895a73b083 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -907,7 +907,8 @@ static MTPData *usb_mtp_get_object_handles(MTPState *s, MTPControl *c,
                                            MTPObject *o)
 {
     MTPData *d = usb_mtp_data_alloc(c);
-    uint32_t i = 0, handles[o->nchildren];
+    uint32_t i = 0;
+    g_autofree uint32_t *handles = g_new(uint32_t, o->nchildren);
     MTPObject *iter;
 
     trace_usb_mtp_op_get_object_handles(s->dev.addr, o->handle, o->path);
-- 
2.31.1

[PULL 6/6] usb: limit combined packets to 1 MiB (CVE-2021-3527)

[Gerd Hoffmann]

usb-host and usb-redirect try to batch bulk transfers by combining many
small usb packets into a single, large transfer request, to reduce the
overhead and improve performance.

This patch adds a size limit of 1 MiB for those combined packets to
restrict the host resources the guest can bind that way.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-Id: <20210503132915.2335822-6-kraxel@redhat.com>
---
 hw/usb/combined-packet.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/usb/combined-packet.c b/hw/usb/combined-packet.c
index 5d57e883dcb5..e56802f89a32 100644
--- a/hw/usb/combined-packet.c
+++ b/hw/usb/combined-packet.c
@@ -171,7 +171,9 @@ void usb_ep_combine_input_packets(USBEndpoint *ep)
         if ((p->iov.size % ep->max_packet_size) != 0 || !p->short_not_ok ||
                 next == NULL ||
                 /* Work around for Linux usbfs bulk splitting + migration */
-                (totalsize == (16 * KiB - 36) && p->int_req)) {
+                (totalsize == (16 * KiB - 36) && p->int_req) ||
+                /* Next package may grow combined package over 1MiB */
+                totalsize > 1 * MiB - ep->max_packet_size) {
             usb_device_handle_data(ep->dev, first);
             assert(first->status == USB_RET_ASYNC);
             if (first->combined) {
-- 
2.31.1

Re: [PULL 4/6] usb/redir: avoid dynamic stack allocation (CVE-2021-3527)

[Remy Noel]

On Wed, May 05, 2021 at 03:07:14PM +0200, Gerd Hoffmann wrote:
>[...]
>diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
>index 17f06f34179a..6a75b0dc4ab2 100644
>--- a/hw/usb/redirect.c
>+++ b/hw/usb/redirect.c
>@@ -620,7 +620,7 @@ static void usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
>                 .endpoint = ep,
>                 .length = p->iov.size
>             };
>-            uint8_t buf[p->iov.size];
>+            g_autofree uint8_t *buf = g_malloc(p->iov.size);
>             /* No id, we look at the ep when receiving a status back */
>             usb_packet_copy(p, buf, p->iov.size);
>             usbredirparser_send_iso_packet(dev->parser, 0, &iso_packet,
>@@ -818,7 +818,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
>         usbredirparser_send_bulk_packet(dev->parser, p->id,
>                                         &bulk_packet, NULL, 0);
>     } else {
>-        uint8_t buf[size];
>+        g_autofree uint8_t *buf = g_malloc(size);
>         usb_packet_copy(p, buf, size);

Won't this allows us to malloc then write an arbitrary amount of heap 
memory, allowing a guest driver to abort the qemu ?

Remy

Re: [PULL 4/6] usb/redir: avoid dynamic stack allocation (CVE-2021-3527)

[Gerd Hoffmann]

On Wed, May 05, 2021 at 03:29:10PM +0200, Remy Noel wrote:
> On Wed, May 05, 2021 at 03:07:14PM +0200, Gerd Hoffmann wrote:
> > [...]
> > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> > index 17f06f34179a..6a75b0dc4ab2 100644
> > --- a/hw/usb/redirect.c
> > +++ b/hw/usb/redirect.c
> > @@ -620,7 +620,7 @@ static void usbredir_handle_iso_data(USBRedirDevice *dev, USBPacket *p,
> >                 .endpoint = ep,
> >                 .length = p->iov.size
> >             };
> > -            uint8_t buf[p->iov.size];
> > +            g_autofree uint8_t *buf = g_malloc(p->iov.size);
> >             /* No id, we look at the ep when receiving a status back */
> >             usb_packet_copy(p, buf, p->iov.size);
> >             usbredirparser_send_iso_packet(dev->parser, 0, &iso_packet,
> > @@ -818,7 +818,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
> >         usbredirparser_send_bulk_packet(dev->parser, p->id,
> >                                         &bulk_packet, NULL, 0);
> >     } else {
> > -        uint8_t buf[size];
> > +        g_autofree uint8_t *buf = g_malloc(size);
> >         usb_packet_copy(p, buf, size);
> 
> Won't this allows us to malloc then write an arbitrary amount of heap
> memory, allowing a guest driver to abort the qemu ?

Crashing qemu is not as easy as with stack allocation, but yes, unbound
allocation is still there.  Need to figure some way to limit this in
xhci without breaking things.

Or maybe use g_try_malloc() and catch allocation failures.

Alternatively we could add a usbredirparser_send_bulk_packet_iov()
function to usbredir so we can just pass on the iov and don't need a
temporary buffer in the first place.

Not sure yet what the best way forward is.
Comments (and other ideas) are welcome.

take care,
  Gerd

Re: [PULL 0/6] Usb 20210505 patches

[Peter Maydell]

On Wed, 5 May 2021 at 14:09, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit 15106f7dc3290ff3254611f265849a314a93eb0e:
>
>   Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-hex-20210502' into staging (2021-05-02 16:23:05 +0100)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/usb-20210505-pull-request
>
> for you to fetch changes up to 05a40b172e4d691371534828078be47e7fff524c:
>
>   usb: limit combined packets to 1 MiB (CVE-2021-3527) (2021-05-05 15:06:01 +0200)
>
> ----------------------------------------------------------------
> usb: fix some memory allocation issues (CVE-2021-3527).
> usb: add stubs, allow building without usb subsystem.
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.

-- PMM