1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/usb/dev-uas: Fix guest-triggerable heap OOB access

Thomas Huth posted 1 patch 1 week, 1 day ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260716193321.256391-1-thuth@redhat.com
There is a newer version of this series
hw/usb/dev-uas.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
[PATCH] hw/usb/dev-uas: Fix guest-triggerable heap OOB access
Posted by Thomas Huth 1 week, 1 day ago 
From: Thomas Huth <thuth@redhat.com>

The stream ID is under control of the guest, so we must not use
it for indexing into the status3[] array without checking it for
being in range.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3612
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3986
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/usb/dev-uas.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 8576dfec96f..7df58cdeee7 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -383,8 +383,15 @@ static void usb_uas_send_status_bh(void *opaque)
 
 static void usb_uas_queue_status(UASDevice *uas, UASStatus *st, int length)
 {
-    USBPacket *p = uas_using_streams(uas) ?
-        uas->status3[st->stream] : uas->status2;
+    USBPacket *p = NULL;
+
+    if (!uas_using_streams(uas)) {
+        p = uas->status2;
+    } else if (st->stream <= UAS_MAX_STREAMS) {
+        p = uas->status3[st->stream];
+    } else {
+        warn_report_once(TYPE_USB_UAS ": bad stream ID 0x%x", st->stream);
+    }
 
     st->length += length;
     QTAILQ_INSERT_TAIL(&uas->results, st, next);
-- 
2.55.0
Re: [PATCH] hw/usb/dev-uas: Fix guest-triggerable heap OOB access
Posted by Peter Maydell 1 week, 1 day ago 
On Thu, 16 Jul 2026 at 20:33, Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> The stream ID is under control of the guest, so we must not use
> it for indexing into the status3[] array without checking it for
> being in range.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3612
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3986
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/usb/dev-uas.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index 8576dfec96f..7df58cdeee7 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -383,8 +383,15 @@ static void usb_uas_send_status_bh(void *opaque)
>
>  static void usb_uas_queue_status(UASDevice *uas, UASStatus *st, int length)
>  {
> -    USBPacket *p = uas_using_streams(uas) ?
> -        uas->status3[st->stream] : uas->status2;
> +    USBPacket *p = NULL;
> +
> +    if (!uas_using_streams(uas)) {
> +        p = uas->status2;
> +    } else if (st->stream <= UAS_MAX_STREAMS) {
> +        p = uas->status3[st->stream];
> +    } else {
> +        warn_report_once(TYPE_USB_UAS ": bad stream ID 0x%x", st->stream);
> +    }
>
>      st->length += length;
>      QTAILQ_INSERT_TAIL(&uas->results, st, next);

Doesn't this just postpone the problem? This function puts the
UASStatus struct (with the out-of-range stream value) oto the
uas->results queue. Later on we will end up in the
usb_uas_send_status_bh function, which picks USBStatus structs
off the queue and uses the same "assume st->stream is in bounds"
logic for finding a USBPacket for them.

Also, once we put this UASStatus into the results queue we
will never take it off again -- the code for handling reads
on the status pipe will only remove and process UASStatus
entries where st->stream matches the stream ID used for
the read on the status pipe (which must be in-bounds, we
check that).

The copy of the USB attached storage spec I found is not very
clear about error handling. But I think there is no way for us
to report back the "invalid stream ID" case. So I think:

(1) in usb_uas_command() we should check the tag for being
in bounds first, before any other error check; if it is not
then LOG_GUEST_ERROR and return (do not call
usb_uas_queue_fake_sense(); have comment about why not)

(2) is usb_uas_queue_status() and usb_uas_send_status_bh(),
assert() that st->stream is in bounds before using it

thanks
-- PMM
Re: [PATCH] hw/usb/dev-uas: Fix guest-triggerable heap OOB access
Posted by Thomas Huth 5 days, 2 hours ago 
On 17/07/2026 13.05, Peter Maydell wrote:
> On Thu, 16 Jul 2026 at 20:33, Thomas Huth <thuth@redhat.com> wrote:
>>
>> From: Thomas Huth <thuth@redhat.com>
>>
>> The stream ID is under control of the guest, so we must not use
>> it for indexing into the status3[] array without checking it for
>> being in range.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3612
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3986
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/usb/dev-uas.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
>> index 8576dfec96f..7df58cdeee7 100644
>> --- a/hw/usb/dev-uas.c
>> +++ b/hw/usb/dev-uas.c
>> @@ -383,8 +383,15 @@ static void usb_uas_send_status_bh(void *opaque)
>>
>>   static void usb_uas_queue_status(UASDevice *uas, UASStatus *st, int length)
>>   {
>> -    USBPacket *p = uas_using_streams(uas) ?
>> -        uas->status3[st->stream] : uas->status2;
>> +    USBPacket *p = NULL;
>> +
>> +    if (!uas_using_streams(uas)) {
>> +        p = uas->status2;
>> +    } else if (st->stream <= UAS_MAX_STREAMS) {
>> +        p = uas->status3[st->stream];
>> +    } else {
>> +        warn_report_once(TYPE_USB_UAS ": bad stream ID 0x%x", st->stream);
>> +    }
>>
>>       st->length += length;
>>       QTAILQ_INSERT_TAIL(&uas->results, st, next);
> 
> Doesn't this just postpone the problem? This function puts the
> UASStatus struct (with the out-of-range stream value) oto the
> uas->results queue. Later on we will end up in the
> usb_uas_send_status_bh function, which picks USBStatus structs
> off the queue and uses the same "assume st->stream is in bounds"
> logic for finding a USBPacket for them.
> 
> Also, once we put this UASStatus into the results queue we
> will never take it off again -- the code for handling reads
> on the status pipe will only remove and process UASStatus
> entries where st->stream matches the stream ID used for
> the read on the status pipe (which must be in-bounds, we
> check that).
> 
> The copy of the USB attached storage spec I found is not very
> clear about error handling. But I think there is no way for us
> to report back the "invalid stream ID" case. So I think:
> 
> (1) in usb_uas_command() we should check the tag for being
> in bounds first, before any other error check; if it is not
> then LOG_GUEST_ERROR and return (do not call
> usb_uas_queue_fake_sense(); have comment about why not)
> 
> (2) is usb_uas_queue_status() and usb_uas_send_status_bh(),
> assert() that st->stream is in bounds before using it
Yes, after staring at the code for a while, I think you're right. I'll send 
a v2 with those changes.

  Thomas
Re: [PATCH] hw/usb/dev-uas: Fix guest-triggerable heap OOB access
Posted by Thomas Huth 1 week, 1 day ago 
On 16/07/2026 21.33, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> The stream ID is under control of the guest, so we must not use
> it for indexing into the status3[] array without checking it for
> being in range.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3612
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3986

As requested by the reporter in the bug ticket:
Reported-by: Tristan Madani <tristan@talencesecurity.com>

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/usb/dev-uas.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index 8576dfec96f..7df58cdeee7 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -383,8 +383,15 @@ static void usb_uas_send_status_bh(void *opaque)
>   
>   static void usb_uas_queue_status(UASDevice *uas, UASStatus *st, int length)
>   {
> -    USBPacket *p = uas_using_streams(uas) ?
> -        uas->status3[st->stream] : uas->status2;
> +    USBPacket *p = NULL;
> +
> +    if (!uas_using_streams(uas)) {
> +        p = uas->status2;
> +    } else if (st->stream <= UAS_MAX_STREAMS) {
> +        p = uas->status3[st->stream];
> +    } else {
> +        warn_report_once(TYPE_USB_UAS ": bad stream ID 0x%x", st->stream);
> +    }
>   
>       st->length += length;
>       QTAILQ_INSERT_TAIL(&uas->results, st, next);

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.