Usb 20180507 patches

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

Posted by Gerd Hoffmann 7 years, 9 months ago

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

Posted by Peter Maydell 7 years, 8 months ago

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