Usb 20180507 patches

GIT PULL Superseded Gerd Hoffmann 3 participants 14 messages May 7, 2018 – May 18, 2018

lore

Share & tools

lore

Gerd Hoffmann

author

2018-05-07 09:45 UTC #0 lore raw

The following changes since commit c8b7e627b4269a3bc3ae41d9f420547a47e6d9b9:

Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2018-05-04' into staging (2018-05-04 14:42:46 +0100)

are available in the git repository at:

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

for you to fetch changes up to 3280ea8edede3814553aa19fa27a58daedd48ad9:

usb-host: skip open on pending postload bh (2018-05-07 11:10:42 +0200)

----------------------------------------------------------------
usb: fixes for mtp and host.

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

Bandan Das (2):
usb-mtp: Add some NULL checks for issues pointed out by coverity
usb-mtp: Unconditionally check for the readonly bit

Gerd Hoffmann (1):
usb-host: skip open on pending postload bh

 hw/usb/dev-mtp.c     | 15 ++++++++-------
 hw/usb/host-libusb.c |  7 +++++++
 2 files changed, 15 insertions(+), 7 deletions(-)

Signature · 1 line

2.9.3

Gerd Hoffmann

author

2018-05-07 09:45 UTC #1 lore raw

[Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity

From: Bandan Das <[email protected]>

CID 1390578: In usb_mtp_write_metadata, parent can never be NULL but just in case, add an assert
CID 1390592: Check for o->format only if o !=NULL
CID 1390604: Check s->data_out != NULL in usb_mtp_handle_data

Signed-off-by Bandan Das <[email protected]>

Message-id: [email protected]

Signed-off-by Gerd Hoffmann <[email protected]>

---
 hw/usb/dev-mtp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

hw/usb/dev-mtp.c +3 -3

index 6ecf70a79b..24cff640c0 100644
@@ -1446,8 +1446,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
             if (o == NULL) {
                 usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE, c->trans,
                                      0, 0, 0, 0);
-            }
-            if (o->format != FMT_ASSOCIATION) {
+            } else if (o->format != FMT_ASSOCIATION) {
                 usb_mtp_queue_result(s, RES_INVALID_PARENT_OBJECT, c->trans,
                                      0, 0, 0, 0);
             }

@@ -1660,6 +1659,7 @@ static void usb_mtp_write_metadata(MTPState *s)
     uint32_t next_handle = s->next_handle;
 
     assert(!s->write_pending);
+    assert(p != NULL);
 
     utf16_to_str(dataset->length, dataset->filename, filename);

@@ -1838,7 +1838,7 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)
             p->status = USB_RET_STALL;
             return;
         }
-        if (s->data_out && !s->data_out->first) {
+        if ((s->data_out != NULL) && !s->data_out->first) {
             container_type = TYPE_DATA;
         } else {
             usb_packet_copy(p, &container, sizeof(container));

Signature · 1 line

2.9.3

Peter Maydell

2018-05-15 18:50 UTC #2 lore raw

Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity

Quoted 8 lines · Gerd Hoffmann

From: Bandan Das <[email protected]>
CID 1390578: In usb_mtp_write_metadata, parent can never be NULL but just in case, add an assert
CID 1390592: Check for o->format only if o !=NULL
CID 1390604: Check s->data_out != NULL in usb_mtp_handle_data

hw/usb/dev-mtp.c static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)

Quoted 9 lines

hw/usb/dev-mtp.c +1 -2

@@ -1838,7 +1838,7 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)
             p->status = USB_RET_STALL;
             return;
         }
-        if (s->data_out && !s->data_out->first) {
+        if ((s->data_out != NULL) && !s->data_out->first) {
             container_type = TYPE_DATA;
         } else {
             usb_packet_copy(p, &container, sizeof(container));
--

I just noticed this, but this isn't actually changing the logic in this function: "s->data_out" and "s->data_out != NULL" are exactly the same test. So this won't change CID 1390604.

Looking back at my previous email on this, it looks like I managed to completely misinterpret the code and/or what coverity is talking about, which is probably the source of the confusion. Let me try again...

In the function usb_mtp_handle_data() EP_DATA_OUT case, we have:
(1) a check for whether s->data_out is NULL, which implies that it might be NULL sometimes
(2) some time later, a call to usb_mtp_get_data() which is

     not protected by a test for whether s->data_out is NULL

and if s->data_out is NULL at point (2) then usb_mtp_get_data() will crash.

Obviously the code path that goes through "containe_type = TYPE_DATA" must have s->data_out being non-NULL. But in the else branch of that, can the container_type we get via usb_packet_copy() ever be TYPE_DATA ? (It's not clear to me whether that comes from a trusted or untrusted source.)

If this is a "can't happen" situation we can mark it as a false positive in coverity.

thanks
-- PMM

Bandan Das

2018-05-17 21:42 UTC #3 lore raw

Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity

Quoted 8 lines · Peter Maydell

Quoted 6 lines · Gerd Hoffmann
From: Bandan Das <[email protected]>
CID 1390578: In usb_mtp_write_metadata, parent can never be NULL but just in case, add an assert
CID 1390592: Check for o->format only if o !=NULL
CID 1390604: Check s->data_out != NULL in usb_mtp_handle_data

Quoted 14 lines

hw/usb/dev-mtp.c static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)
Quoted 10 lines
hw/usb/dev-mtp.c +1 -2
@@ -1838,7 +1838,7 @@ static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)
             p->status = USB_RET_STALL;
             return;
         }
-        if (s->data_out && !s->data_out->first) {
+        if ((s->data_out != NULL) && !s->data_out->first) {
             container_type = TYPE_DATA;
         } else {
             usb_packet_copy(p, &container, sizeof(container));
--
I just noticed this, but this isn't actually changing the logic in this function: "s->data_out" and "s->data_out != NULL" are exactly the same test. So this won't change CID 1390604.

Right, indeed they are. I am unfamiliar with coverity and just incorrectly assumed that somehow it's assuming the test for NULL can return false.

Quoted 14 lines

Looking back at my previous email on this, it looks like I managed to completely misinterpret the code and/or what coverity is talking about, which is probably the source of the confusion. Let me try again...
In the function usb_mtp_handle_data() EP_DATA_OUT case, we have:
(1) a check for whether s->data_out is NULL, which implies that it might be NULL sometimes
(2) some time later, a call to usb_mtp_get_data() which is
     not protected by a test for whether s->data_out is NULL
and if s->data_out is NULL at point (2) then usb_mtp_get_data() will crash.

Quoted 8 lines

Obviously the code path that goes through "containe_type = TYPE_DATA" must have s->data_out being non-NULL. But in the else branch of that, can the container_type we get via usb_packet_copy() ever be TYPE_DATA ? (It's not clear to me whether that comes from a trusted or untrusted source.)
If this is a "can't happen" situation we can mark it as a false positive in coverity.

The protocol ofcourse won't let this happen but the guest can't be trusted. It can easily send a packet with TYPE_DATA without sending OBJECT_INFO first that allocates memory for data_out. I will submit a fix.

Thanks for clearing out the confusion.

Bandan

Quoted 2 lines

thanks
-- PMM

Bandan Das

2018-05-18 18:23 UTC #4 lore raw

PATCH 1/1 usb-mtp: Assert on suspicious TYPE_DATA packet from initiator

CID 1390604
If the initiator sends a packet with TYPE_DATA set without initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data can trip on a null s->data_out.

Signed-off-by Bandan Das <[email protected]>

---
 hw/usb/dev-mtp.c | 1 +
 1 file changed, 1 insertion(+)

hw/usb/dev-mtp.c +1 -0

index 3d59fe4944..905e025d7f 100644
@@ -1696,6 +1696,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
     uint64_t dlen;
     uint32_t data_len = p->iov.size;
 
+    assert(d != NULL);
     if (d->first) {
         /* Total length of incoming data */
         d->length = cpu_to_le32(container->length) - sizeof(mtp_container);

Signature · 1 line

2.14.3

Peter Maydell

2018-05-18 18:26 UTC #5 lore raw

Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator

Quoted 7 lines · Bandan Das

CID 1390604
If the initiator sends a packet with TYPE_DATA set without initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data can trip on a null s->data_out.
Signed-off-by Bandan Das <[email protected]>

I think you said this can be provoked by the guest? Misbehaving or malicious guests should never be able to provoke assertions.

Quoted 17 lines

---
 hw/usb/dev-mtp.c | 1 +
 1 file changed, 1 insertion(+)

hw/usb/dev-mtp.c +1 -1

index 3d59fe4944..905e025d7f 100644
@@ -1696,6 +1696,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
     uint64_t dlen;
     uint32_t data_len = p->iov.size;
 
+    assert(d != NULL);
     if (d->first) {
         /* Total length of incoming data */
         d->length = cpu_to_le32(container->length) - sizeof(mtp_container);
--

Quoted 1 line

2.14.3

thanks
-- PMM

Bandan Das

2018-05-18 18:38 UTC #6 lore raw

Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator

Quoted 10 lines · Peter Maydell

Quoted 7 lines · Bandan Das
CID 1390604
If the initiator sends a packet with TYPE_DATA set without initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data can trip on a null s->data_out.
Signed-off-by Bandan Das <[email protected]>
I think you said this can be provoked by the guest?

Yes, this can only be initated by the guest as far as I understand.

Quoted 2 lines

Misbehaving or malicious guests should never be able to provoke assertions.

I am not sure, I thought it's better to kill a misbehaving guest rather than silently letting it run. Anyway, it's possible to send a No_Valid_ObjectInfo as well and we wouldn't have to mark it as a false positive either.

Bandan

Quoted 21 lines

Quoted 17 lines

---
 hw/usb/dev-mtp.c | 1 +
 1 file changed, 1 insertion(+)

hw/usb/dev-mtp.c +1 -1

index 3d59fe4944..905e025d7f 100644
@@ -1696,6 +1696,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
     uint64_t dlen;
     uint32_t data_len = p->iov.size;
 
+    assert(d != NULL);
     if (d->first) {
         /* Total length of incoming data */
         d->length = cpu_to_le32(container->length) - sizeof(mtp_container);
--

Quoted 1 line

2.14.3

thanks
-- PMM

Peter Maydell

2018-05-18 18:49 UTC #7 lore raw

Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator

Quoted 16 lines · Bandan Das

Quoted 10 lines · Peter Maydell
Quoted 7 lines · Bandan Das
CID 1390604
If the initiator sends a packet with TYPE_DATA set without initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data can trip on a null s->data_out.
Signed-off-by Bandan Das <[email protected]>
I think you said this can be provoked by the guest?
Yes, this can only be initated by the guest as far as I understand.

Quoted 7 lines

Quoted 2 lines
Misbehaving or malicious guests should never be able to provoke assertions.
I am not sure, I thought it's better to kill a misbehaving guest rather than silently letting it run. Anyway, it's possible to send a No_Valid_ObjectInfo as well and we wouldn't have to mark it as a false positive either.

Broadly speaking, where we're emulating hardware we should do what the hardware does when the guest does wrong things to it. A real server doesn't suddenly vanish leaving behind a message saying "assertion failed" :-)

thanks
-- PMM

Bandan Das

2018-05-18 19:04 UTC #8 lore raw

Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator

Quoted 25 lines · Peter Maydell

Quoted 16 lines · Bandan Das
Quoted 10 lines · Peter Maydell
Quoted 7 lines · Bandan Das
CID 1390604
If the initiator sends a packet with TYPE_DATA set without initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data can trip on a null s->data_out.
Signed-off-by Bandan Das <[email protected]>
I think you said this can be provoked by the guest?
Yes, this can only be initated by the guest as far as I understand.
Quoted 7 lines
Quoted 2 lines
Misbehaving or malicious guests should never be able to provoke assertions.
I am not sure, I thought it's better to kill a misbehaving guest rather than silently letting it run. Anyway, it's possible to send a No_Valid_ObjectInfo as well and we wouldn't have to mark it as a false positive either.

Quoted 4 lines

Broadly speaking, where we're emulating hardware we should do what the hardware does when the guest does wrong things to it. A real server doesn't suddenly vanish leaving behind a message saying "assertion failed" :-)

Posted v2 and agree with you! Especially, since the protocol does specify the case where something like this can happen.

Thanks for reviewing,
Bandan

Quoted 2 lines

thanks
-- PMM

Bandan Das

2018-05-18 18:49 UTC #9 lore raw

PATCH v2 1/1 usb-mtp: Return error on suspicious TYPE_DATA packet from initiator

In reply to Bandan Das

CID 1390604
If the initiator sends a packet with TYPE_DATA set without initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data can trip on a null s->data_out.

Signed-off-by Bandan Das <[email protected]>

---
 hw/usb/dev-mtp.c | 5 +++++
 1 file changed, 5 insertions(+)

hw/usb/dev-mtp.c +5 -0

index 3d59fe4944..28384ea3b0 100644
@@ -1696,6 +1696,11 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container,
     uint64_t dlen;
     uint32_t data_len = p->iov.size;
 
+    if (!d) {
+            usb_mtp_queue_result(s, RES_INVALID_OBJECTINFO, 0,
+                                 0, 0, 0, 0);
+            return;
+    }
     if (d->first) {
         /* Total length of incoming data */
         d->length = cpu_to_le32(container->length) - sizeof(mtp_container);

Signature · 1 line

2.14.3

Bandan Das

2018-05-18 18:25 UTC #10 lore raw

Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity

In reply to Bandan Das

Quoted 2 lines · Bandan Das

Quoted 2 lines
If this is a "can't happen" situation we can mark it as a false positive in coverity.

I posted a patch with an assert added in usb_mtp_get_data. I believe CID 1390604 can be marked as a false positive.

Thanks,
Bandan

Quoted 11 lines

The protocol ofcourse won't let this happen but the guest can't be trusted. It can easily send a packet with TYPE_DATA without sending OBJECT_INFO first that allocates memory for data_out. I will submit a fix.
Thanks for clearing out the confusion.
Bandan
Quoted 2 lines
thanks
-- PMM

Gerd Hoffmann

author

2018-05-07 09:45 UTC #11 lore raw

[Qemu-devel] [PULL 2/3] usb-mtp: Unconditionally check for the readonly bit

In reply to Gerd Hoffmann

From: Bandan Das <[email protected]>

Currently, it's only being checked if desc is NULL and so write support breaks upon specifying desc

Signed-off-by Bandan Das <[email protected]>

Message-id: [email protected]

Signed-off-by Gerd Hoffmann <[email protected]>

---
 hw/usb/dev-mtp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

hw/usb/dev-mtp.c +5 -4

index 24cff640c0..3d59fe4944 100644
@@ -1948,16 +1948,17 @@ static void usb_mtp_realize(USBDevice *dev, Error **errp)
             return;
         }
         s->desc = strrchr(s->root, '/');
-        /* Mark store as RW */
-        if (!s->readonly) {
-            s->flags |= (1 << MTP_FLAG_WRITABLE);
-        }
         if (s->desc && s->desc[0]) {
             s->desc = g_strdup(s->desc + 1);
         } else {
             s->desc = g_strdup("none");
         }
     }
+    /* Mark store as RW */
+    if (!s->readonly) {
+        s->flags |= (1 << MTP_FLAG_WRITABLE);
+    }
+
 }
 
 static const VMStateDescription vmstate_usb_mtp = {

Signature · 1 line

2.9.3

Gerd Hoffmann

author

2018-05-07 09:45 UTC #12 lore raw

[Qemu-devel] [PULL 3/3] usb-host: skip open on pending postload bh

In reply to Gerd Hoffmann

usb-host emulates a device unplug after live migration, because the device state is unknown and unplug/replug makes sure the guest re-initializes the device into a working state. This can't be done in post-load though, so post-load just schedules a bottom half which executes after vmload is complete.

It can happen that the device autoscan timer hits the race window between scheduling and running the bottom half, which in turn can triggers an assert().

Fix that issue by just ignoring the usb_host_open() call in case the bottom half didn't execute yet.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1572851

Signed-off-by Gerd Hoffmann <[email protected]>

Message-id: [email protected]

---
 hw/usb/host-libusb.c | 7 +++++++
 1 file changed, 7 insertions(+)

hw/usb/host-libusb.c +7 -0

index dc0a8fe295..f31e9cbbb8 100644
@@ -102,6 +102,7 @@ struct USBHostDevice {
     /* callbacks & friends */
     QEMUBH                           *bh_nodev;
     QEMUBH                           *bh_postld;
+    bool                             bh_postld_pending;
     Notifier                         exit;
 
     /* request queues */

@@ -870,6 +871,10 @@ static int usb_host_open(USBHostDevice *s, libusb_device *dev)
     int rc;
     Error *local_err = NULL;
 
+    if (s->bh_postld_pending) {
+        return -1;
+    }
+
     trace_usb_host_open_started(bus_num, addr);
 
     if (s->dh != NULL) {

@@ -1528,6 +1533,7 @@ static void usb_host_post_load_bh(void *opaque)
     if (udev->attached) {
         usb_device_detach(udev);
     }
+    dev->bh_postld_pending = false;
     usb_host_auto_check(NULL);
 }

@@ -1539,6 +1545,7 @@ static int usb_host_post_load(void *opaque, int version_id)
         dev->bh_postld = qemu_bh_new(usb_host_post_load_bh, dev);
     }
     qemu_bh_schedule(dev->bh_postld);
+    dev->bh_postld_pending = true;
     return 0;
 }

Signature · 1 line

2.9.3

Peter Maydell

Applied

2018-05-08 13:23 UTC #13 lore raw

In reply to Gerd Hoffmann

Quoted 21 lines · Gerd Hoffmann

The following changes since commit c8b7e627b4269a3bc3ae41d9f420547a47e6d9b9:
Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2018-05-04' into staging (2018-05-04 14:42:46 +0100)
are available in the git repository at:
git://git.kraxel.org/qemu tags/usb-20180507-pull-request
for you to fetch changes up to 3280ea8edede3814553aa19fa27a58daedd48ad9:
usb-host: skip open on pending postload bh (2018-05-07 11:10:42 +0200)
----------------------------------------------------------------
usb: fixes for mtp and host.
----------------------------------------------------------------
Bandan Das (2):
usb-mtp: Add some NULL checks for issues pointed out by coverity
usb-mtp: Unconditionally check for the readonly bit

Quoted 6 lines

Gerd Hoffmann (1):
usb-host: skip open on pending postload bh
 hw/usb/dev-mtp.c     | 15 ++++++++-------
 hw/usb/host-libusb.c |  7 +++++++
 2 files changed, 15 insertions(+), 7 deletions(-)

Applied, thanks.

-- PMM