From f8224fb0faec9f4184b29b8158534536e6580748 Mon Sep 17 00:00:00 2001 From: Juergen Gross Date: Thu, 6 Dec 2018 14:39:23 +0100 Subject: pvusb: set max grants only in initialise Don't call xen_be_set_max_grant_refs() in usbback_alloc(), as the gnttabdev pointer won't be initialised yet. The call can easily be moved to usbback_connect(). Signed-off-by: Juergen Gross Message-id: 20181206133923.30105-1-jgross@suse.com Signed-off-by: Gerd Hoffmann --- hw/usb/xen-usb.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'hw') diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c index 5b2e21ed18..f5d5c91094 100644 --- a/hw/usb/xen-usb.c +++ b/hw/usb/xen-usb.c @@ -860,10 +860,14 @@ static int usbback_connect(struct XenDevice *xendev) struct usbif_conn_sring *conn_sring; int urb_ring_ref; int conn_ring_ref; - unsigned int i; + unsigned int i, max_grants; TR_BUS(xendev, "start\n"); + /* max_grants: for each request and for the rings (request and connect). */ + max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; + xen_be_set_max_grant_refs(xendev, max_grants); + usbif = container_of(xendev, struct usbback_info, xendev); if (xenstore_read_fe_int(xendev, "urb-ring-ref", &urb_ring_ref)) { @@ -1005,7 +1009,7 @@ static void usbback_alloc(struct XenDevice *xendev) { struct usbback_info *usbif; USBPort *p; - unsigned int i, max_grants; + unsigned int i; usbif = container_of(xendev, struct usbback_info, xendev); @@ -1021,10 +1025,6 @@ static void usbback_alloc(struct XenDevice *xendev) QTAILQ_INIT(&usbif->req_free_q); QSIMPLEQ_INIT(&usbif->hotplug_q); usbif->bh = qemu_bh_new(usbback_bh, usbif); - - /* max_grants: for each request and for the rings (request and connect). */ - max_grants = USBIF_MAX_SEGMENTS_PER_REQUEST * USB_URB_RING_SIZE + 2; - xen_be_set_max_grant_refs(xendev, max_grants); } static int usbback_free(struct XenDevice *xendev) -- cgit v1.2.3 From 5621d0453c60ce4fc104a9795791d6402386c3b3 Mon Sep 17 00:00:00 2001 From: linzhecheng Date: Fri, 30 Nov 2018 14:47:00 +0800 Subject: usb-host: reset and close libusb_device_handle before qemu exit we should perform these things as same as usb_host_close. Signed-off-by: linzhecheng Message-id: 20181130064700.5984-1-linzhecheng@huawei.com [ kraxel: whitespace fixup ] Signed-off-by: Gerd Hoffmann --- hw/usb/host-libusb.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'hw') diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c index b6602ded4e..833250a886 100644 --- a/hw/usb/host-libusb.c +++ b/hw/usb/host-libusb.c @@ -988,7 +988,9 @@ static void usb_host_exit_notifier(struct Notifier *n, void *data) if (s->dh) { usb_host_release_interfaces(s); + libusb_reset_device(s->dh); usb_host_attach_kernel(s); + libusb_close(s->dh); } } -- cgit v1.2.3 From b7d3a7e1a8830af78e71952e82f186b12b70ff1f Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 26 Nov 2018 11:08:36 +0100 Subject: ehci: fix fetch qtd race The token field contains the (guest-filled) state of the qtd, which indicates whenever the other fields are valid or not. So make sure we read the token first, otherwise we may end up with an stale next pointer: (1) ehci reads next (2) guest writes next (3) guest writes token (4) ehci reads token (5) ehci operates with stale next. Typical effect is that qemu doesn't notice that the guest appends new qtds to the end of the queue. Looks like the usb device stopped responding. Linux can recover from that, but leaves a message in the kernel log that it did reset the usb device in question. Signed-off-by: Gerd Hoffmann Message-id: 20181126100836.8805-1-kraxel@redhat.com --- hw/usb/hcd-ehci.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index e5acfc5ba5..8d44d483df 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1783,9 +1783,17 @@ static int ehci_state_fetchqtd(EHCIQueue *q) EHCIqtd qtd; EHCIPacket *p; int again = 1; + uint32_t addr; - if (get_dwords(q->ehci, NLPTR_GET(q->qtdaddr), (uint32_t *) &qtd, - sizeof(EHCIqtd) >> 2) < 0) { + addr = NLPTR_GET(q->qtdaddr); + if (get_dwords(q->ehci, addr + 8, &qtd.token, 1) < 0) { + return 0; + } + barrier(); + if (get_dwords(q->ehci, addr + 0, &qtd.next, 1) < 0 || + get_dwords(q->ehci, addr + 4, &qtd.altnext, 1) < 0 || + get_dwords(q->ehci, addr + 12, qtd.bufptr, + ARRAY_SIZE(qtd.bufptr)) < 0) { return 0; } ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd); -- cgit v1.2.3 From bab9df35ce73d1c8e19a37e2737717ea1c984dc1 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 13 Dec 2018 13:25:11 +0100 Subject: usb-mtp: use O_NOFOLLOW and O_CLOEXEC. Open files and directories with O_NOFOLLOW to avoid symlinks attacks. While being at it also add O_CLOEXEC. usb-mtp only handles regular files and directories and ignores everything else, so users should not see a difference. Because qemu ignores symlinks, carrying out a successful symlink attack requires swapping an existing file or directory below rootdir for a symlink and winning the race against the inotify notification to qemu. Fixes: CVE-2018-16872 Cc: Prasad J Pandit Cc: Bandan Das Reported-by: Michael Hanselmann Signed-off-by: Gerd Hoffmann Reviewed-by: Michael Hanselmann Message-id: 20181213122511.13853-1-kraxel@redhat.com --- hw/usb/dev-mtp.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'hw') diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 100b7171f4..36c43b8c20 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -653,13 +653,18 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o) { struct dirent *entry; DIR *dir; + int fd; if (o->have_children) { return; } o->have_children = true; - dir = opendir(o->path); + fd = open(o->path, O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW); + if (fd < 0) { + return; + } + dir = fdopendir(fd); if (!dir) { return; } @@ -1007,7 +1012,7 @@ static MTPData *usb_mtp_get_object(MTPState *s, MTPControl *c, trace_usb_mtp_op_get_object(s->dev.addr, o->handle, o->path); - d->fd = open(o->path, O_RDONLY); + d->fd = open(o->path, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); if (d->fd == -1) { usb_mtp_data_free(d); return NULL; @@ -1031,7 +1036,7 @@ static MTPData *usb_mtp_get_partial_object(MTPState *s, MTPControl *c, c->argv[1], c->argv[2]); d = usb_mtp_data_alloc(c); - d->fd = open(o->path, O_RDONLY); + d->fd = open(o->path, O_RDONLY | O_CLOEXEC | O_NOFOLLOW); if (d->fd == -1) { usb_mtp_data_free(d); return NULL; @@ -1658,7 +1663,7 @@ static void usb_mtp_write_data(MTPState *s) 0, 0, 0, 0); goto done; } - d->fd = open(path, O_CREAT | O_WRONLY, mask); + d->fd = open(path, O_CREAT | O_WRONLY | O_CLOEXEC | O_NOFOLLOW, mask); if (d->fd == -1) { usb_mtp_queue_result(s, RES_STORE_FULL, d->trans, 0, 0, 0, 0); -- cgit v1.2.3 From 90c1a74271ce4667d16eeca575dfa78a6c7d465c Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Thu, 13 Dec 2018 22:37:06 +0000 Subject: usb-mtp: Limit filename to object information size The filename length in MTP metadata is specified by the guest. By trusting it directly it'd theoretically be possible to get the host to write memory parts outside the filename buffer into a filename. In practice though there are usually NUL bytes stopping the string operations. Also use the opportunity to not assign the filename member twice. Signed-off-by: Michael Hanselmann Message-id: ab70659d8d5c580bdf150a5f7d5cc60c8e374ffc.1544740018.git.public@hansmi.ch [ kraxel: codestyle fix: break a long line ] Signed-off-by: Gerd Hoffmann --- hw/usb/dev-mtp.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'hw') diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 36c43b8c20..6098005cd4 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -1710,7 +1710,7 @@ free: s->write_pending = false; } -static void usb_mtp_write_metadata(MTPState *s) +static void usb_mtp_write_metadata(MTPState *s, uint64_t dlen) { MTPData *d = s->data_out; ObjectInfo *dataset = (ObjectInfo *)d->data; @@ -1722,7 +1722,9 @@ static void usb_mtp_write_metadata(MTPState *s) assert(!s->write_pending); assert(p != NULL); - filename = utf16_to_str(dataset->length, dataset->filename); + filename = utf16_to_str(MIN(dataset->length, + dlen - offsetof(ObjectInfo, filename)), + dataset->filename); if (strchr(filename, '/')) { usb_mtp_queue_result(s, RES_PARAMETER_NOT_SUPPORTED, d->trans, @@ -1738,7 +1740,6 @@ static void usb_mtp_write_metadata(MTPState *s) s->dataset.filename = filename; s->dataset.format = dataset->format; s->dataset.size = dataset->size; - s->dataset.filename = filename; s->write_pending = true; if (s->dataset.format == FMT_ASSOCIATION) { @@ -1807,7 +1808,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, if (d->offset == d->length) { /* The operation might have already failed */ if (!s->result) { - usb_mtp_write_metadata(s); + usb_mtp_write_metadata(s, dlen); } usb_mtp_data_free(s->data_out); s->data_out = NULL; -- cgit v1.2.3