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

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

[Gerd Hoffmann]

The following changes since commit 4f818e7b7f8ecb5c166d093b8859fec2ddeca2ef:

  Update version for v3.1.0-rc5 release (2018-12-06 17:07:12 +0000)

are available in the git repository at:

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

for you to fetch changes up to 90c1a74271ce4667d16eeca575dfa78a6c7d465c:

  usb-mtp: Limit filename to object information size (2018-12-14 08:57:17 +0100)

----------------------------------------------------------------
usb: fixes for mtp, ehci, usb-host and pvusb (xen).

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

Gerd Hoffmann (2):
  ehci: fix fetch qtd race
  usb-mtp: use O_NOFOLLOW and O_CLOEXEC.

Juergen Gross (1):
  pvusb: set max grants only in initialise

Michael Hanselmann (1):
  usb-mtp: Limit filename to object information size

linzhecheng (1):
  usb-host: reset and close libusb_device_handle before qemu exit

 hw/usb/dev-mtp.c     | 22 ++++++++++++++--------
 hw/usb/hcd-ehci.c    | 12 ++++++++++--
 hw/usb/host-libusb.c |  2 ++
 hw/usb/xen-usb.c     | 12 ++++++------
 4 files changed, 32 insertions(+), 16 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/5] pvusb: set max grants only in initialise

[Gerd Hoffmann]

From: Juergen Gross <jgross@suse.com>

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 <jgross@suse.com>
Message-id: 20181206133923.30105-1-jgross@suse.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/xen-usb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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)
-- 
2.9.3

[Qemu-devel] [PULL 2/5] usb-host: reset and close libusb_device_handle before qemu exit

[Gerd Hoffmann]

From: linzhecheng <linzhecheng@huawei.com>

we should perform these things as same as usb_host_close.

Signed-off-by: linzhecheng <linzhecheng@huawei.com>
Message-id: 20181130064700.5984-1-linzhecheng@huawei.com

[ kraxel: whitespace fixup ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/host-libusb.c | 2 ++
 1 file changed, 2 insertions(+)

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);
     }
 }
 
-- 
2.9.3

[Qemu-devel] [PULL 3/5] ehci: fix fetch qtd race

[Gerd Hoffmann]

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 <kraxel@redhat.com>
Message-id: 20181126100836.8805-1-kraxel@redhat.com
---
 hw/usb/hcd-ehci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

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

[Qemu-devel] [PULL 4/5] usb-mtp: use O_NOFOLLOW and O_CLOEXEC.

[Gerd Hoffmann]

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 <ppandit@redhat.com>
Cc: Bandan Das <bsd@redhat.com>
Reported-by: Michael Hanselmann <public@hansmi.ch>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Michael Hanselmann <public@hansmi.ch>
Message-id: 20181213122511.13853-1-kraxel@redhat.com
---
 hw/usb/dev-mtp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

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

[Qemu-devel] [PULL 5/5] usb-mtp: Limit filename to object information size

[Gerd Hoffmann]

From: Michael Hanselmann <public@hansmi.ch>

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 <public@hansmi.ch>
Message-id: ab70659d8d5c580bdf150a5f7d5cc60c8e374ffc.1544740018.git.public@hansmi.ch

[ kraxel: codestyle fix: break a long line ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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

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

[Peter Maydell]

On Fri, 14 Dec 2018 at 10:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit 4f818e7b7f8ecb5c166d093b8859fec2ddeca2ef:
>
>   Update version for v3.1.0-rc5 release (2018-12-06 17:07:12 +0000)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/usb-20181214-pull-request
>
> for you to fetch changes up to 90c1a74271ce4667d16eeca575dfa78a6c7d465c:
>
>   usb-mtp: Limit filename to object information size (2018-12-14 08:57:17 +0100)
>
> ----------------------------------------------------------------
> usb: fixes for mtp, ehci, usb-host and pvusb (xen).
>
> ----------------------------------------------------------------


Applied, thanks.

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

-- PMM