[Qemu-devel] [PULL 0/3] Usb 20180507 patches

[Qemu-devel] [PULL 0/3] Usb 20180507 patches

[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

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

-- 
2.9.3

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

[Gerd Hoffmann]

From: Bandan Das <bsd@redhat.com>

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 <bsd@redhat.com>
Message-id: 20180503192028.14353-2-bsd@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 6ecf70a79b..24cff640c0 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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));
-- 
2.9.3

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

[Peter Maydell]

On 7 May 2018 at 10:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
> From: Bandan Das <bsd@redhat.com>
>
> 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

> @@ -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

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

[Bandan Das]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 May 2018 at 10:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> From: Bandan Das <bsd@redhat.com>
>>
>> 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
>
>> @@ -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.

> 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.

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

> thanks
> -- PMM

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

[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 <bsd@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 3d59fe4944..905e025d7f 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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);
-- 
2.14.3

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

[Peter Maydell]

On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote:
>
> 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 <bsd@redhat.com>

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

> ---
>  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 3d59fe4944..905e025d7f 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -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);
> --
> 2.14.3

thanks
-- PMM

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

[Bandan Das]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote:
>>
>> 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 <bsd@redhat.com>
>
> 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.

> 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

>> ---
>>  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 3d59fe4944..905e025d7f 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -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);
>> --
>> 2.14.3
>
> thanks
> -- PMM

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

[Peter Maydell]

On 18 May 2018 at 19:38, Bandan Das <bsd@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote:
>>>
>>> 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 <bsd@redhat.com>
>>
>> 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.
>
>> 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

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

[Bandan Das]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 18 May 2018 at 19:38, Bandan Das <bsd@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote:
>>>>
>>>> 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 <bsd@redhat.com>
>>>
>>> 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.
>>
>>> 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" :-)

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

Thanks for reviewing,
Bandan

> thanks
> -- PMM

[Qemu-devel] [PATCH v2] usb-mtp: Return error on suspicious TYPE_DATA packet from initiator

[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 <bsd@redhat.com>
---
 hw/usb/dev-mtp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 3d59fe4944..28384ea3b0 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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);
-- 
2.14.3

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

[Bandan Das]

Bandan Das <bsd@redhat.com> writes:

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

> 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
>
>> thanks
>> -- PMM

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

[Gerd Hoffmann]

From: Bandan Das <bsd@redhat.com>

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

Signed-off-by: Bandan Das <bsd@redhat.com>
Message-id: 20180503192028.14353-3-bsd@redhat.com
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 24cff640c0..3d59fe4944 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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 = {
-- 
2.9.3

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

[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 <kraxel@redhat.com>
Message-id: 20180503062932.17233-1-kraxel@redhat.com
---
 hw/usb/host-libusb.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index dc0a8fe295..f31e9cbbb8 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -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;
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/3] Usb 20180507 patches

[Peter Maydell]

On 7 May 2018 at 10:44, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 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(-)

Applied, thanks.

-- PMM