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

[Thomas Huth <thuth@redhat.com>]

From: Thomas Huth <thuth@redhat.com>

The stream ID is under control of the guest, and some spots in the
code currently use it for indexing into the status3[] array without
checking it for being in range first, so the code accesses the heap
beyond the limit of the status3 array.

Since our status delivery code depends on having a valid stream ID,
we must not try to generate a fake sense code in this situation.
Simply log a guest error and return early in usb_uas_command().

And to make sure that we really cannot access the status3[] array
beyond its limit anymore, add some assert() statements in the
affected functions, too.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3612
Reported-by: Reported-by: huntr bubble
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3986
Reported-by: Tristan Madani <tristan@talencesecurity.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2: Refuse illegal tags already in usb_uas_command()

 hw/usb/dev-uas.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 8576dfec96f..963c0433b38 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -362,6 +362,7 @@ static void usb_uas_send_status_bh(void *opaque)
 
     while ((st = QTAILQ_FIRST(&uas->results)) != NULL) {
         if (uas_using_streams(uas)) {
+            assert(st->stream <= UAS_MAX_STREAMS);
             p = uas->status3[st->stream];
             uas->status3[st->stream] = NULL;
         } else {
@@ -383,8 +384,14 @@ 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;
+
+    if (uas_using_streams(uas)) {
+        assert(st->stream <= UAS_MAX_STREAMS);
+        p = uas->status3[st->stream];
+    } else {
+        p = uas->status2;
+    }
 
     st->length += length;
     QTAILQ_INSERT_TAIL(&uas->results, st, next);
@@ -700,14 +707,22 @@ static void usb_uas_command(UASDevice *uas, uas_iu *iu)
     uint16_t tag = be16_to_cpu(iu->hdr.tag);
     size_t cdb_len = sizeof(iu->command.cdb) + iu->command.add_cdb_length;
 
+    if (uas_using_streams(uas) && tag > UAS_MAX_STREAMS) {
+        /*
+         * Our status delivery only works with valid tags, so in case the
+         * stream ID is out of bounds, we have to return immediately here
+         * without sending a fake sense_code_INVALID_TAG to the guest.
+         */
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "invalid tag 0x%x for USB UAS command\n", tag);
+        return;
+    }
+
     if (iu->command.add_cdb_length > 0) {
         qemu_log_mask(LOG_UNIMP, "additional adb length not yet supported\n");
         goto unsupported_len;
     }
 
-    if (uas_using_streams(uas) && tag > UAS_MAX_STREAMS) {
-        goto invalid_tag;
-    }
     req = usb_uas_find_request(uas, tag);
     if (req) {
         goto overlapped_tag;
@@ -744,10 +759,6 @@ unsupported_len:
     usb_uas_queue_fake_sense(uas, tag, sense_code_INVALID_PARAM_VALUE);
     return;
 
-invalid_tag:
-    usb_uas_queue_fake_sense(uas, tag, sense_code_INVALID_TAG);
-    return;
-
 overlapped_tag:
     usb_uas_queue_fake_sense(uas, tag, sense_code_OVERLAPPED_COMMANDS);
     return;
-- 
2.55.0