[Qemu-devel] [PATCH 0/5] uas emulation fixes

[Qemu-devel] [PATCH 0/5] uas emulation fixes

[Hans de Goede]

Hi Gerd et al,

Here is a bunch of uas fixes, they are hopefully self
explanatory...

Regards,

Hans

[Qemu-devel] [PATCH 1/5] scsi: Add 2 new sense codes needed by uas

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/scsi/scsi-bus.c     | 10 ++++++++++
 include/hw/scsi/scsi.h |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 24ec52f..2414696 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1293,6 +1293,11 @@ const struct SCSISense sense_code_ILLEGAL_REQ_REMOVAL_PREVENTED = {
     .key = ILLEGAL_REQUEST, .asc = 0x53, .ascq = 0x02
 };
 
+/* Illegal request, Invalid Transfer Tag */
+const struct SCSISense sense_code_INVALID_TAG = {
+    .key = ILLEGAL_REQUEST, .asc = 0x4b, .ascq = 0x01
+};
+
 /* Command aborted, I/O process terminated */
 const struct SCSISense sense_code_IO_ERROR = {
     .key = ABORTED_COMMAND, .asc = 0x00, .ascq = 0x06
@@ -1308,6 +1313,11 @@ const struct SCSISense sense_code_LUN_FAILURE = {
     .key = ABORTED_COMMAND, .asc = 0x3e, .ascq = 0x01
 };
 
+/* Command aborted, Overlapped Commands Attempted */
+const struct SCSISense sense_code_OVERLAPPED_COMMANDS = {
+    .key = ABORTED_COMMAND, .asc = 0x4e, .ascq = 0x00
+};
+
 /* Unit attention, Capacity data has changed */
 const struct SCSISense sense_code_CAPACITY_CHANGED = {
     .key = UNIT_ATTENTION, .asc = 0x2a, .ascq = 0x09
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 76f6ac2..bf6da3d 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -199,12 +199,16 @@ extern const struct SCSISense sense_code_SAVING_PARAMS_NOT_SUPPORTED;
 extern const struct SCSISense sense_code_INCOMPATIBLE_FORMAT;
 /* Illegal request, medium removal prevented */
 extern const struct SCSISense sense_code_ILLEGAL_REQ_REMOVAL_PREVENTED;
+/* Illegal request, Invalid Transfer Tag */
+extern const struct SCSISense sense_code_INVALID_TAG;
 /* Command aborted, I/O process terminated */
 extern const struct SCSISense sense_code_IO_ERROR;
 /* Command aborted, I_T Nexus loss occurred */
 extern const struct SCSISense sense_code_I_T_NEXUS_LOSS;
 /* Command aborted, Logical Unit failure */
 extern const struct SCSISense sense_code_LUN_FAILURE;
+/* Command aborted, Overlapped Commands Attempted */
+extern const struct SCSISense sense_code_OVERLAPPED_COMMANDS;
 /* LUN not ready, Capacity data has changed */
 extern const struct SCSISense sense_code_CAPACITY_CHANGED;
 /* LUN not ready, Medium not present */
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/5] uas: Only use report iu-s for task_mgmt status reporting

[Hans de Goede]

Regular scsi cmds should always report their status using a sense-iu, using
the sense code to report any errors.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-uas.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 70ed2d1..36a75b2 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -420,6 +420,24 @@ static void usb_uas_queue_sense(UASRequest *req, uint8_t status)
     usb_uas_queue_status(req->uas, st, len);
 }
 
+static void usb_uas_queue_fake_sense(UASDevice *uas, uint16_t tag,
+                                     struct SCSISense sense)
+{
+    UASStatus *st = usb_uas_alloc_status(uas, UAS_UI_SENSE, tag);
+    int len, slen = 0;
+
+    st->status.sense.status = CHECK_CONDITION;
+    st->status.sense.status_qualifier = cpu_to_be16(0);
+    st->status.sense.sense_data[0] = 0x70;
+    st->status.sense.sense_data[2] = sense.key;
+    st->status.sense.sense_data[7] = 10;
+    st->status.sense.sense_data[12] = sense.asc;
+    st->status.sense.sense_data[13] = sense.ascq;
+    slen = 18;
+    len = sizeof(uas_ui_sense) - sizeof(st->status.sense.sense_data) + slen;
+    usb_uas_queue_status(uas, st, len);
+}
+
 static void usb_uas_queue_read_ready(UASRequest *req)
 {
     UASStatus *st = usb_uas_alloc_status(req->uas, UAS_UI_READ_READY,
@@ -672,8 +690,9 @@ static void usb_uas_command(UASDevice *uas, uas_ui *ui)
 {
     UASRequest *req;
     uint32_t len;
+    uint16_t tag = be16_to_cpu(ui->hdr.tag);
 
-    req = usb_uas_find_request(uas, be16_to_cpu(ui->hdr.tag));
+    req = usb_uas_find_request(uas, tag);
     if (req) {
         goto overlapped_tag;
     }
@@ -706,16 +725,11 @@ static void usb_uas_command(UASDevice *uas, uas_ui *ui)
     return;
 
 overlapped_tag:
-    usb_uas_queue_response(uas, req->tag, UAS_RC_OVERLAPPED_TAG, 0);
+    usb_uas_queue_fake_sense(uas, tag, sense_code_OVERLAPPED_COMMANDS);
     return;
 
 bad_target:
-    /*
-     * FIXME: Seems to upset linux, is this wrong?
-     * NOTE: Happens only with no scsi devices at the bus, not sure
-     *       this is a valid UAS setup in the first place.
-     */
-    usb_uas_queue_response(uas, req->tag, UAS_RC_INVALID_INFO_UNIT, 0);
+    usb_uas_queue_fake_sense(uas, tag, sense_code_LUN_NOT_SUPPORTED);
     g_free(req);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/5] uas: Fix / cleanup usb_uas_task error handling

[Hans de Goede]

-The correct error if we cannot find the dev is INCORRECT_LUN rather then
 INVALID_INFO_UNIT
-Move the device not found check to the top so we only need to do it once
-Remove the dev->lun != lun checks, dev is returned by scsi_device_find
 which searches by lun, so this will never trigger

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-uas.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 36a75b2..12d79ef 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -746,17 +746,14 @@ static void usb_uas_task(UASDevice *uas, uas_ui *ui)
     if (req) {
         goto overlapped_tag;
     }
+    if (dev == NULL) {
+        goto incorrect_lun;
+    }
 
     switch (ui->task.function) {
     case UAS_TMF_ABORT_TASK:
         task_tag = be16_to_cpu(ui->task.task_tag);
         trace_usb_uas_tmf_abort_task(uas->dev.addr, tag, task_tag);
-        if (dev == NULL) {
-            goto bad_target;
-        }
-        if (dev->lun != lun) {
-            goto incorrect_lun;
-        }
         req = usb_uas_find_request(uas, task_tag);
         if (req && req->dev == dev) {
             scsi_req_cancel(req->req);
@@ -766,12 +763,6 @@ static void usb_uas_task(UASDevice *uas, uas_ui *ui)
 
     case UAS_TMF_LOGICAL_UNIT_RESET:
         trace_usb_uas_tmf_logical_unit_reset(uas->dev.addr, tag, lun);
-        if (dev == NULL) {
-            goto bad_target;
-        }
-        if (dev->lun != lun) {
-            goto incorrect_lun;
-        }
         qdev_reset_all(&dev->qdev);
         usb_uas_queue_response(uas, tag, UAS_RC_TMF_COMPLETE, 0);
         break;
@@ -787,11 +778,6 @@ overlapped_tag:
     usb_uas_queue_response(uas, req->tag, UAS_RC_OVERLAPPED_TAG, 0);
     return;
 
-bad_target:
-    /* FIXME: correct?  [see long comment in usb_uas_command()] */
-    usb_uas_queue_response(uas, tag, UAS_RC_INVALID_INFO_UNIT, 0);
-    return;
-
 incorrect_lun:
     usb_uas_queue_response(uas, tag, UAS_RC_INCORRECT_LUN, 0);
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/5] uas: Streams are numbered 1-y, rather then 0-x

[Hans de Goede]

It is easier to simply make the arrays one larger, rather then
substracting one everywhere.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-uas.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 12d79ef..70f41d3 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -122,8 +122,8 @@ struct UASDevice {
     UASRequest                *dataout2;
 
     /* usb 3.0 only */
-    USBPacket                 *data3[UAS_MAX_STREAMS];
-    USBPacket                 *status3[UAS_MAX_STREAMS];
+    USBPacket                 *data3[UAS_MAX_STREAMS + 1];
+    USBPacket                 *status3[UAS_MAX_STREAMS + 1];
 };
 
 struct UASRequest {
@@ -666,7 +666,7 @@ static void usb_uas_cancel_io(USBDevice *dev, USBPacket *p)
         return;
     }
     if (uas_using_streams(uas)) {
-        for (i = 0; i < UAS_MAX_STREAMS; i++) {
+        for (i = 0; i <= UAS_MAX_STREAMS; i++) {
             if (uas->status3[i] == p) {
                 uas->status3[i] = NULL;
                 return;
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/5] uas: Bounds check tags when using streams

[Hans de Goede]

Disallow the guest to cause us to address the data3 and status3 arrays
out of bounds.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-uas.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 70f41d3..5884035 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -692,6 +692,9 @@ static void usb_uas_command(UASDevice *uas, uas_ui *ui)
     uint32_t len;
     uint16_t tag = be16_to_cpu(ui->hdr.tag);
 
+    if (uas_using_streams(uas) && tag > UAS_MAX_STREAMS) {
+        goto invalid_tag;
+    }
     req = usb_uas_find_request(uas, tag);
     if (req) {
         goto overlapped_tag;
@@ -724,6 +727,10 @@ static void usb_uas_command(UASDevice *uas, uas_ui *ui)
     }
     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;
@@ -742,6 +749,9 @@ static void usb_uas_task(UASDevice *uas, uas_ui *ui)
     UASRequest *req;
     uint16_t task_tag;
 
+    if (uas_using_streams(uas) && tag > UAS_MAX_STREAMS) {
+        goto invalid_tag;
+    }
     req = usb_uas_find_request(uas, be16_to_cpu(ui->hdr.tag));
     if (req) {
         goto overlapped_tag;
@@ -774,6 +784,10 @@ static void usb_uas_task(UASDevice *uas, uas_ui *ui)
     }
     return;
 
+invalid_tag:
+    usb_uas_queue_response(uas, tag, UAS_RC_INVALID_INFO_UNIT, 0);
+    return;
+
 overlapped_tag:
     usb_uas_queue_response(uas, req->tag, UAS_RC_OVERLAPPED_TAG, 0);
     return;
-- 
1.8.3.1