1. Patchew
  2. QEMU
  3. View series

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

Bandan Das posted 1 patch 7 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/jpga7swygak.fsf_-_@linux.bootlegged.copy
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
hw/usb/dev-mtp.c | 1 +
1 file changed, 1 insertion(+)
[Qemu-devel] [PATCH] usb-mtp: Assert on suspicious TYPE_DATA packet from initiator
Posted by Bandan Das 7 years, 5 months ago 

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
Posted by Peter Maydell 7 years, 5 months ago 
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
Posted by Bandan Das 7 years, 5 months ago 
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
Posted by Peter Maydell 7 years, 5 months ago 
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
Posted by Bandan Das 7 years, 5 months ago 
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
Posted by Bandan Das 7 years, 5 months ago 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.