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

On 17/07/2026 13.05, Peter Maydell wrote:
> On Thu, 16 Jul 2026 at 20:33, Thomas Huth <[email protected]> wrote:
>>
>> From: Thomas Huth <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>   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