From 3fd2092fd11b9e4220a08eca0663cc59178a6c3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 21 Dec 2018 13:41:15 +0000 Subject: hw/usb: fix mistaken de-initialization of CCID state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In previous commit: commit 7dea29e4af17fc1d27478de9f8ea38144deac54a Author: Li Qiang 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 Signed-off-by: Daniel P. Berrangé Reviewed-by: Michael Tokarev Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Message-id: 20181221134115.27973-1-berrange@redhat.com Signed-off-by: Gerd Hoffmann --- 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 25976ed..e0457d3 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: -- cgit v1.1 From f30815390adb1ec153327c3832ab378e8bce9808 Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Mon, 7 Jan 2019 17:51:40 +0000 Subject: usb: drop unnecessary usb_device_post_load checks 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 #include #include #include 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 Message-Id: <20190107175117.23769-1-jonathan.davies@nutanix.com> Signed-off-by: Gerd Hoffmann --- hw/usb/bus.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hw/usb/bus.c b/hw/usb/bus.c index bf796d6..6fffab7 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; } -- cgit v1.1 From 8e3759ef04dbd87ad21502e78b068d6473c6b5d9 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Thu, 3 Jan 2019 05:26:05 -0800 Subject: usb: dev-mtp: fix memory leak in error path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spotted by Coverity: CID 1397074 Fixes: c52d46e041b Signed-off-by: Li Qiang Reviewed-by: Philippe Mathieu-Daudé Message-id: 20190103132605.49476-1-liq3ea@163.com Signed-off-by: Gerd Hoffmann --- 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 6098005..b19b576 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; } -- cgit v1.1 From 114529f79ec37261dda7e3c7846a04a5bfc212bd Mon Sep 17 00:00:00 2001 From: Hongbo Zhang Date: Sat, 29 Dec 2018 18:00:57 +0800 Subject: hw/usb: Add generic sys-bus EHCI controller 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 Message-id: 1546077657-22637-1-git-send-email-hongbo.zhang@linaro.org Signed-off-by: Gerd Hoffmann --- hw/usb/hcd-ehci-sysbus.c | 17 +++++++++++++++++ hw/usb/hcd-ehci.h | 1 + 2 files changed, 18 insertions(+) diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c index 3b83beb..331faf8 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); diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h index 0bc364b..cd30b5d 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" -- cgit v1.1 From efce3175fd572e4d98af0b50ae9cb8d7b808f034 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 17 Dec 2018 17:32:38 +0100 Subject: usb: move ehci_create_ich9_with_companions to hw/i386 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 Message-id: 1545064358-4601-1-git-send-email-pbonzini@redhat.com Signed-off-by: Gerd Hoffmann --- hw/i386/pc_q35.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++ hw/usb/hcd-ehci-pci.c | 53 --------------------------------------------------- include/hw/usb.h | 2 -- 3 files changed, 53 insertions(+), 55 deletions(-) diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 0a3f3f1..f8efae8 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 8c0fc53..69abbf7 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; -} diff --git a/include/hw/usb.h b/include/hw/usb.h index a5080ad..4961405 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) */ -- cgit v1.1