[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues poi
From: |
Bandan Das |
Subject: |
Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity |
Date: |
Thu, 17 May 2018 17:41:59 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Peter Maydell <address@hidden> writes:
> On 7 May 2018 at 10:44, Gerd Hoffmann <address@hidden> wrote:
>> From: Bandan Das <address@hidden>
>>
>> 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] [PULL 0/3] Usb 20180507 patches, Gerd Hoffmann, 2018/05/07
- [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity, Gerd Hoffmann, 2018/05/07
- Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity, Peter Maydell, 2018/05/15
- Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity,
Bandan Das <=
- [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator, Bandan Das, 2018/05/18
- Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator, Peter Maydell, 2018/05/18
- Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator, Bandan Das, 2018/05/18
- Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator, Peter Maydell, 2018/05/18
- Re: [Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator, Bandan Das, 2018/05/18
- [Qemu-devel] [PATCH v2] usb-mtp: Return error on suspicious TYPE_DATA packet from initiator, Bandan Das, 2018/05/18
- Re: [Qemu-devel] [PULL 1/3] usb-mtp: Add some NULL checks for issues pointed out by coverity, Bandan Das, 2018/05/18
[Qemu-devel] [PULL 2/3] usb-mtp: Unconditionally check for the readonly bit, Gerd Hoffmann, 2018/05/07
[Qemu-devel] [PULL 3/3] usb-host: skip open on pending postload bh, Gerd Hoffmann, 2018/05/07
Re: [Qemu-devel] [PULL 0/3] Usb 20180507 patches, Peter Maydell, 2018/05/08