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
