[Qemu-devel] [PULL 0/5] Usb 20190108 patches

[Qemu-devel] [PULL 0/5] Usb 20190108 patches

[Gerd Hoffmann]

The following changes since commit e59dbbac0364344a3ad84c3497a98c56003d3fb8:

  Merge remote-tracking branch 'remotes/philmd/tags/fw_cfg-20190104-pull-request' into staging (2019-01-04 18:23:13 +0000)

are available in the git repository at:

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

for you to fetch changes up to efce3175fd572e4d98af0b50ae9cb8d7b808f034:

  usb: move ehci_create_ich9_with_companions to hw/i386 (2019-01-08 12:43:41 +0100)

----------------------------------------------------------------
usb: generic sysbus ehci, bugfixes.

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

Daniel P. Berrangé (1):
  hw/usb: fix mistaken de-initialization of CCID state

Hongbo Zhang (1):
  hw/usb: Add generic sys-bus EHCI controller

Jonathan Davies (1):
  usb: drop unnecessary usb_device_post_load checks

Li Qiang (1):
  usb: dev-mtp: fix memory leak in error path

Paolo Bonzini (1):
  usb: move ehci_create_ich9_with_companions to hw/i386

 hw/usb/hcd-ehci.h           |  1 +
 include/hw/usb.h            |  2 --
 hw/i386/pc_q35.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++
 hw/usb/bus.c                |  6 -----
 hw/usb/ccid-card-emulated.c |  2 ++
 hw/usb/dev-mtp.c            |  1 +
 hw/usb/hcd-ehci-pci.c       | 53 ---------------------------------------------
 hw/usb/hcd-ehci-sysbus.c    | 17 +++++++++++++++
 8 files changed, 74 insertions(+), 61 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/5] hw/usb: fix mistaken de-initialization of CCID state

[Gerd Hoffmann]

From: Daniel P. Berrangé <berrange@redhat.com>

In previous commit:

  commit 7dea29e4af17fc1d27478de9f8ea38144deac54a
  Author: Li Qiang <liq3ea@gmail.com>
  Date:   Fri Oct 19 03:50:36 2018 -0700

    hw: ccid-card-emulated: cleanup resource when realize in error path

The emulated_realize method was changed so that it jumps to a cleanup
label to de-initialize state upon error. This change failed to ensure
the success path exited the method before this point though. So the
mutexes are always destroyed even in normal operation. The result is
as crashtastic as expected:

$ qemu-system-x86_64 -usb -device usb-ccid,id=ccid0 -device ccid-card-emulated,backend=nss-emulated,id=smartcard0,bus=ccid0.0
qemu-system-x86_64: util/qemu-thread-posix.c:64: qemu_mutex_lock_impl: Assertion `mutex->initialized' failed.
Aborted (core dumped)

Fixes: 7dea29e4af1
Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20181221134115.27973-1-berrange@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/ccid-card-emulated.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 25976ed84f..e0457d305b 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -549,6 +549,8 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
     qemu_thread_create(&card->apdu_thread_id, "ccid/apdu", handle_apdu_thread,
                        card, QEMU_THREAD_JOINABLE);
 
+    return;
+
 out2:
     clean_event_notifier(card);
 out1:
-- 
2.9.3

[Qemu-devel] [PULL 2/5] usb: drop unnecessary usb_device_post_load checks

[Gerd Hoffmann]

From: Jonathan Davies <jonathan.davies@nutanix.com>

In usb_device_post_load, certain values of dev->setup_len or
dev->setup_index can cause -EINVAL to be returned. One example is when
setup_len exceeds 4096, the hard-coded value of sizeof(dev->data_buf).
This can happen through legitimate guest activity and will cause all
subsequent attempts to migrate the guest to fail in vmstate_load_state.

The values of these variables can be set by USB packets originating in
the guest. There are two ways in which they can be set: in
do_token_setup and in do_parameter in hw/usb/core.c.

It is easy to craft a USB packet in a guest that causes do_token_setup
to set setup_len to a value larger than 4096. When this has been done
once, all subsequent attempts to migrate the VM will fail in
usb_device_post_load until the VM is next power-cycled or a
smaller-sized USB packet is sent to the device.

Sample code for achieving this in a VM started with "-device usb-tablet"
running Linux with CONFIG_HIDRAW=y and HID_MAX_BUFFER_SIZE > 4096:

  #include <sys/types.h>
  #include <sys/stat.h>
  #include <fcntl.h>
  #include <unistd.h>

  int main() {
           char buf[4097];
           int fd = open("/dev/hidraw0", O_RDWR|O_NONBLOCK);

           buf[0] = 0x1;
           write(fd, buf, 4097);

           return 0;
  }

When this code is run in the VM, qemu will output:

  usb_generic_handle_packet: ctrl buffer too small (4097 > 4096)

A subsequent attempt to migrate the VM will fail and output the
following on the destination host:

  qemu-kvm: error while loading state for instance 0x0 of device '0000:00:06.7/1/usb-ptr'
  qemu-kvm: load of migration failed: Invalid argument

The idea behind checking the values of setup_len and setup_index before
they are used is correct, but doing it in usb_device_post_load feels
arbitrary, and will cause unnecessary migration failures. Indeed, none
of the commit messages for c60174e8, 9f8e9895 and 719ffe1f justify why
post_load is the right place to do these checks. They correctly point
out that the important thing to protect is the usb_packet_copy.

Instead, the right place to do the checks is in do_token_setup and
do_parameter. Indeed, there are already some checks here. We can examine
each of the disjuncts currently tested in usb_device_post_load to see
whether any need adding to do_token_setup or do_parameter to improve
safety there:

  * dev->setup_index < 0
     - This test is not needed because setup_index is explicitly set to
0 in do_token_setup and do_parameter.

  * dev->setup_len < 0
     - In both do_token_setup and do_parameter, the value of setup_len
is computed by (s->setup_buf[7] << 8) | s->setup_buf[6]. Since
s->setup_buf is a byte array and setup_len is an int32_t, it's
impossible for this arithmetic to set setup_len's top bit, so it can
never be negative.

  * dev->setup_index > dev->setup_len
     - Since setup_index is 0, this is equivalent to the previous test,
so is redundant.

  * dev->setup_len > sizeof(dev->data_buf)
     - This condition is already explicitly checked in both
do_token_setup and do_parameter.

Hence there is no need to bolster the existing checks in do_token_setup
or do_parameter, and we can safely remove these checks from
usb_device_post_load without reducing safety but allowing migrations to
proceed regardless of what USB packets have been generated by the guest.

Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com>
Message-Id: <20190107175117.23769-1-jonathan.davies@nutanix.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/bus.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index bf796d67e6..6fffab7bfa 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -59,12 +59,6 @@ static int usb_device_post_load(void *opaque, int version_id)
     } else {
         dev->attached = true;
     }
-    if (dev->setup_index < 0 ||
-        dev->setup_len < 0 ||
-        dev->setup_index > dev->setup_len ||
-        dev->setup_len > sizeof(dev->data_buf)) {
-        return -EINVAL;
-    }
     return 0;
 }
 
-- 
2.9.3

[Qemu-devel] [PULL 3/5] usb: dev-mtp: fix memory leak in error path

[Gerd Hoffmann]

From: Li Qiang <liq3ea@163.com>

Spotted by Coverity: CID 1397074

Fixes: c52d46e041b
Signed-off-by: Li Qiang <liq3ea@163.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-id: 20190103132605.49476-1-liq3ea@163.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 6098005cd4..b19b576278 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1729,6 +1729,7 @@ static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen)
     if (strchr(filename, '/')) {
         usb_mtp_queue_result(s, RES_PARAMETER_NOT_SUPPORTED, d->trans,
                              0, 0, 0, 0);
+        g_free(filename);
         return;
     }
 
-- 
2.9.3

[Qemu-devel] [PULL 4/5] hw/usb: Add generic sys-bus EHCI controller

[Gerd Hoffmann]

From: Hongbo Zhang <hongbo.zhang@linaro.org>

This patch introduces a new system bus generic EHCI controller.
For the system bus EHCI controller, we've already had "xlnx",
"exynos4210", "tegra2", "ppc4xx" and "fusbh200", they are specific and
only suitable for their own platforms, platforms such as an Arm server,
may need a generic system bus EHCI controller, this patch creates it,
and the kernel driver ehci_platform.c works well on it.

Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org>
Message-id: 1546077657-22637-1-git-send-email-hongbo.zhang@linaro.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.h        |  1 +
 hw/usb/hcd-ehci-sysbus.c | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
index 0bc364b286..cd30b5d5e0 100644
--- a/hw/usb/hcd-ehci.h
+++ b/hw/usb/hcd-ehci.h
@@ -342,6 +342,7 @@ typedef struct EHCIPCIState {
 
 
 #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
+#define TYPE_PLATFORM_EHCI "platform-ehci-usb"
 #define TYPE_EXYNOS4210_EHCI "exynos4210-ehci-usb"
 #define TYPE_TEGRA2_EHCI "tegra2-ehci-usb"
 #define TYPE_PPC4xx_EHCI "ppc4xx-ehci-usb"
diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
index 3b83beb140..331faf8bc3 100644
--- a/hw/usb/hcd-ehci-sysbus.c
+++ b/hw/usb/hcd-ehci-sysbus.c
@@ -94,6 +94,22 @@ static const TypeInfo ehci_type_info = {
     .class_size    = sizeof(SysBusEHCIClass),
 };
 
+static void ehci_platform_class_init(ObjectClass *oc, void *data)
+{
+    SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    sec->capsbase = 0x0;
+    sec->opregbase = 0x20;
+    set_bit(DEVICE_CATEGORY_USB, dc->categories);
+}
+
+static const TypeInfo ehci_platform_type_info = {
+    .name          = TYPE_PLATFORM_EHCI,
+    .parent        = TYPE_SYS_BUS_EHCI,
+    .class_init    = ehci_platform_class_init,
+};
+
 static void ehci_xlnx_class_init(ObjectClass *oc, void *data)
 {
     SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
@@ -245,6 +261,7 @@ static const TypeInfo ehci_fusbh200_type_info = {
 static void ehci_sysbus_register_types(void)
 {
     type_register_static(&ehci_type_info);
+    type_register_static(&ehci_platform_type_info);
     type_register_static(&ehci_xlnx_type_info);
     type_register_static(&ehci_exynos4210_type_info);
     type_register_static(&ehci_tegra2_type_info);
-- 
2.9.3

[Qemu-devel] [PULL 5/5] usb: move ehci_create_ich9_with_companions to hw/i386

[Gerd Hoffmann]

From: Paolo Bonzini <pbonzini@redhat.com>

This function is only needed when Q35 is in use.  Moving it to
the same file that uses it lets you disable the entire USB
subsystem in x86_64-softmmu.mak; of course doing that will
cause -usb to break horribly, but one thing at a time.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 1545064358-4601-1-git-send-email-pbonzini@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/usb.h      |  2 --
 hw/i386/pc_q35.c      | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/usb/hcd-ehci-pci.c | 53 ---------------------------------------------------
 3 files changed, 53 insertions(+), 55 deletions(-)

diff --git a/include/hw/usb.h b/include/hw/usb.h
index a5080adecc..4961405fa0 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -593,8 +593,6 @@ const char *usb_device_get_product_desc(USBDevice *dev);
 
 const USBDesc *usb_device_get_usb_desc(USBDevice *dev);
 
-int ehci_create_ich9_with_companions(PCIBus *bus, int slot);
-
 /* quirks.c */
 
 /* In bulk endpoints are streaming data sources (iow behave like isoc eps) */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0a3f3f18e4..f8efae8236 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -58,6 +58,59 @@
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
 
+struct ehci_companions {
+    const char *name;
+    int func;
+    int port;
+};
+
+static const struct ehci_companions ich9_1d[] = {
+    { .name = "ich9-usb-uhci1", .func = 0, .port = 0 },
+    { .name = "ich9-usb-uhci2", .func = 1, .port = 2 },
+    { .name = "ich9-usb-uhci3", .func = 2, .port = 4 },
+};
+
+static const struct ehci_companions ich9_1a[] = {
+    { .name = "ich9-usb-uhci4", .func = 0, .port = 0 },
+    { .name = "ich9-usb-uhci5", .func = 1, .port = 2 },
+    { .name = "ich9-usb-uhci6", .func = 2, .port = 4 },
+};
+
+static int ehci_create_ich9_with_companions(PCIBus *bus, int slot)
+{
+    const struct ehci_companions *comp;
+    PCIDevice *ehci, *uhci;
+    BusState *usbbus;
+    const char *name;
+    int i;
+
+    switch (slot) {
+    case 0x1d:
+        name = "ich9-usb-ehci1";
+        comp = ich9_1d;
+        break;
+    case 0x1a:
+        name = "ich9-usb-ehci2";
+        comp = ich9_1a;
+        break;
+    default:
+        return -1;
+    }
+
+    ehci = pci_create_multifunction(bus, PCI_DEVFN(slot, 7), true, name);
+    qdev_init_nofail(&ehci->qdev);
+    usbbus = QLIST_FIRST(&ehci->qdev.child_bus);
+
+    for (i = 0; i < 3; i++) {
+        uhci = pci_create_multifunction(bus, PCI_DEVFN(slot, comp[i].func),
+                                        true, comp[i].name);
+        qdev_prop_set_string(&uhci->qdev, "masterbus", usbbus->name);
+        qdev_prop_set_uint32(&uhci->qdev, "firstport", comp[i].port);
+        qdev_init_nofail(&uhci->qdev);
+    }
+    return 0;
+}
+
 /* PC hardware initialisation */
 static void pc_q35_init(MachineState *machine)
 {
diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 8c0fc53a26..69abbf7b7b 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -230,56 +230,3 @@ static void ehci_pci_register_types(void)
 }
 
 type_init(ehci_pci_register_types)
-
-struct ehci_companions {
-    const char *name;
-    int func;
-    int port;
-};
-
-static const struct ehci_companions ich9_1d[] = {
-    { .name = "ich9-usb-uhci1", .func = 0, .port = 0 },
-    { .name = "ich9-usb-uhci2", .func = 1, .port = 2 },
-    { .name = "ich9-usb-uhci3", .func = 2, .port = 4 },
-};
-
-static const struct ehci_companions ich9_1a[] = {
-    { .name = "ich9-usb-uhci4", .func = 0, .port = 0 },
-    { .name = "ich9-usb-uhci5", .func = 1, .port = 2 },
-    { .name = "ich9-usb-uhci6", .func = 2, .port = 4 },
-};
-
-int ehci_create_ich9_with_companions(PCIBus *bus, int slot)
-{
-    const struct ehci_companions *comp;
-    PCIDevice *ehci, *uhci;
-    BusState *usbbus;
-    const char *name;
-    int i;
-
-    switch (slot) {
-    case 0x1d:
-        name = "ich9-usb-ehci1";
-        comp = ich9_1d;
-        break;
-    case 0x1a:
-        name = "ich9-usb-ehci2";
-        comp = ich9_1a;
-        break;
-    default:
-        return -1;
-    }
-
-    ehci = pci_create_multifunction(bus, PCI_DEVFN(slot, 7), true, name);
-    qdev_init_nofail(&ehci->qdev);
-    usbbus = QLIST_FIRST(&ehci->qdev.child_bus);
-
-    for (i = 0; i < 3; i++) {
-        uhci = pci_create_multifunction(bus, PCI_DEVFN(slot, comp[i].func),
-                                        true, comp[i].name);
-        qdev_prop_set_string(&uhci->qdev, "masterbus", usbbus->name);
-        qdev_prop_set_uint32(&uhci->qdev, "firstport", comp[i].port);
-        qdev_init_nofail(&uhci->qdev);
-    }
-    return 0;
-}
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/5] Usb 20190108 patches

[Peter Maydell]

On Tue, 8 Jan 2019 at 15:58, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit e59dbbac0364344a3ad84c3497a98c56003d3fb8:
>
>   Merge remote-tracking branch 'remotes/philmd/tags/fw_cfg-20190104-pull-request' into staging (2019-01-04 18:23:13 +0000)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/usb-20190108-pull-request
>
> for you to fetch changes up to efce3175fd572e4d98af0b50ae9cb8d7b808f034:
>
>   usb: move ehci_create_ich9_with_companions to hw/i386 (2019-01-08 12:43:41 +0100)
>
> ----------------------------------------------------------------
> usb: generic sysbus ehci, bugfixes.
>
> ----------------------------------------------------------------

Applied, thanks.

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

-- PMM

Re: [Qemu-devel] [PULL 4/5] hw/usb: Add generic sys-bus EHCI controller

[Hongbo Zhang]

Thanks, Gerd.

On Tue, 8 Jan 2019 at 23:54, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> From: Hongbo Zhang <hongbo.zhang@linaro.org>
>
> This patch introduces a new system bus generic EHCI controller.
> For the system bus EHCI controller, we've already had "xlnx",
> "exynos4210", "tegra2", "ppc4xx" and "fusbh200", they are specific and
> only suitable for their own platforms, platforms such as an Arm server,
> may need a generic system bus EHCI controller, this patch creates it,
> and the kernel driver ehci_platform.c works well on it.
>
> Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org>
> Message-id: 1546077657-22637-1-git-send-email-hongbo.zhang@linaro.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/hcd-ehci.h        |  1 +
>  hw/usb/hcd-ehci-sysbus.c | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
> index 0bc364b286..cd30b5d5e0 100644
> --- a/hw/usb/hcd-ehci.h
> +++ b/hw/usb/hcd-ehci.h
> @@ -342,6 +342,7 @@ typedef struct EHCIPCIState {
>
>
>  #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
> +#define TYPE_PLATFORM_EHCI "platform-ehci-usb"
>  #define TYPE_EXYNOS4210_EHCI "exynos4210-ehci-usb"
>  #define TYPE_TEGRA2_EHCI "tegra2-ehci-usb"
>  #define TYPE_PPC4xx_EHCI "ppc4xx-ehci-usb"
> diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
> index 3b83beb140..331faf8bc3 100644
> --- a/hw/usb/hcd-ehci-sysbus.c
> +++ b/hw/usb/hcd-ehci-sysbus.c
> @@ -94,6 +94,22 @@ static const TypeInfo ehci_type_info = {
>      .class_size    = sizeof(SysBusEHCIClass),
>  };
>
> +static void ehci_platform_class_init(ObjectClass *oc, void *data)
> +{
> +    SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    sec->capsbase = 0x0;
> +    sec->opregbase = 0x20;
> +    set_bit(DEVICE_CATEGORY_USB, dc->categories);
> +}
> +
> +static const TypeInfo ehci_platform_type_info = {
> +    .name          = TYPE_PLATFORM_EHCI,
> +    .parent        = TYPE_SYS_BUS_EHCI,
> +    .class_init    = ehci_platform_class_init,
> +};
> +
>  static void ehci_xlnx_class_init(ObjectClass *oc, void *data)
>  {
>      SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> @@ -245,6 +261,7 @@ static const TypeInfo ehci_fusbh200_type_info = {
>  static void ehci_sysbus_register_types(void)
>  {
>      type_register_static(&ehci_type_info);
> +    type_register_static(&ehci_platform_type_info);
>      type_register_static(&ehci_xlnx_type_info);
>      type_register_static(&ehci_exynos4210_type_info);
>      type_register_static(&ehci_tegra2_type_info);
> --
> 2.9.3
>