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 thanks -- PMM
