[Qemu-devel] [PATCH 0/4] scsi: miscellaneous fixes

[Qemu-devel] [PATCH 0/4] scsi: miscellaneous fixes

[Paolo Bonzini]

The most important part is a fix for use-after-free that I found while
testing CD-ROM eject.

Paolo Bonzini (4):
  scsi-disk: fail READ CAPACITY if LBA != 0 but PMI == 0
  scsi-disk: do not complete requests twice
  scsi-disk: bump SCSIRequest reference count until aio completion runs
  scsi-disk: fix retrying a flush

 hw/scsi-disk.c |   38 +++++++++++++++++++++++++++++---------
 2 files changed, 30 insertions(+), 10 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/4] scsi-disk: fail READ CAPACITY if LBA != 0 but PMI == 0

[Paolo Bonzini]

Tested by the Windows Logo Kit SCSI Compliance test. From SBC-3, paragraph
5.25: "The LOGICAL BLOCK ADDRESS field shall be set to zero if the PMI
bit is set to zero. If the PMI bit is set to zero and the LOGICAL BLOCK
ADDRESS field is not set to zero, then the device server shall terminate
the command with CHECK CONDITION status with the sense key set to ILLEGAL
REQUEST and the additional sense code set to INVALID FIELD IN CDB".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   12 ++++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index d9fa8f7..4757a02 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1160,8 +1160,12 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
         /* The normal LEN field for this command is zero.  */
         memset(outbuf, 0, 8);
         bdrv_get_geometry(s->bs, &nb_sectors);
-        if (!nb_sectors)
+        if (!nb_sectors) {
             goto not_ready;
+        }
+        if ((req->cmd.buf[8] & 1) == 0 && req->cmd.lba) {
+            goto illegal_request;
+        }
         nb_sectors /= s->cluster_size;
         /* Returned value is the address of the last sector.  */
         nb_sectors--;
@@ -1206,8 +1210,12 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r)
             DPRINTF("SAI READ CAPACITY(16)\n");
             memset(outbuf, 0, req->cmd.xfer);
             bdrv_get_geometry(s->bs, &nb_sectors);
-            if (!nb_sectors)
+            if (!nb_sectors) {
                 goto not_ready;
+            }
+            if ((req->cmd.buf[14] & 1) == 0 && req->cmd.lba) {
+                goto illegal_request;
+            }
             nb_sectors /= s->cluster_size;
             /* Returned value is the address of the last sector.  */
             nb_sectors--;
-- 
1.7.6

[Qemu-devel] [PATCH 2/4] scsi-disk: do not complete requests twice

[Paolo Bonzini]

When scsi_handle_rw_error reports a CHECK CONDITION code, the
owner should not call scsi_req_complete.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 4757a02..6497655 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -230,6 +230,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
 
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RSTATE_IO_ERROR);
+        return 1;
     } else {
         switch (error) {
         case ENOMEDIUM:
@@ -246,8 +247,8 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
             break;
         }
         bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
+        return 0;
     }
-    return 1;
 }
 
 static void scsi_write_complete(void * opaque, int ret)
-- 
1.7.6

[Qemu-devel] [PATCH 3/4] scsi-disk: bump SCSIRequest reference count until aio completion runs

[Paolo Bonzini]

In some cases a request may be canceled before the completion callback
runs.  Keep a reference to the request between starting an AIO operation,
and let scsi_*_complete remove it.

Since scsi_handle_rw_error returns whether something else has to be done
for the request by the caller, it makes sense to transfer ownership of
the ref to scsi_handle_rw_error when it returns 1; scsi_dma_restart_bh
will then free the reference after restarting the operation.

This is reproducible by doing an "eject -f" during an installer's media
test, using the lsi adapter.  The resulting "ABORT" message causes the
request to be canceled and freed before the read completes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 6497655..d6f2345 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -139,6 +139,7 @@ static void scsi_read_complete(void * opaque, int ret)
 
     if (ret) {
         if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
+            /* Leave in ref for scsi_dma_restart_bh.  */
             return;
         }
     }
@@ -149,6 +150,7 @@ static void scsi_read_complete(void * opaque, int ret)
     r->sector += n;
     r->sector_count -= n;
     scsi_req_data(&r->req, r->qiov.size);
+    scsi_req_unref(&r->req);
 }
 
 static void scsi_flush_complete(void * opaque, int ret)
@@ -163,11 +165,13 @@ static void scsi_flush_complete(void * opaque, int ret)
 
     if (ret < 0) {
         if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_FLUSH)) {
+            /* Leave in ref for scsi_dma_restart_bh.  */
             return;
         }
     }
 
     scsi_req_complete(&r->req, GOOD);
+    scsi_req_unref(&r->req);
 }
 
 /* Read more data from scsi device into buffer.  */
@@ -202,6 +206,9 @@ static void scsi_read_data(SCSIRequest *req)
     if (s->tray_open) {
         scsi_read_complete(r, -ENOMEDIUM);
     }
+
+    /* Save a ref for scsi_read_complete, in case r is canceled.  */
+    scsi_req_ref(&r->req);
     n = scsi_init_iovec(r);
     bdrv_acct_start(s->bs, &r->acct, n * BDRV_SECTOR_SIZE, BDRV_ACCT_READ);
     r->req.aiocb = bdrv_aio_readv(s->bs, r->sector, &r->qiov, n,
@@ -278,6 +285,7 @@ static void scsi_write_complete(void * opaque, int ret)
         DPRINTF("Write complete tag=0x%x more=%d\n", r->req.tag, r->qiov.size);
         scsi_req_data(&r->req, r->qiov.size);
     }
+    scsi_req_unref(&r->req);
 }
 
 static void scsi_write_data(SCSIRequest *req)
@@ -295,6 +303,8 @@ static void scsi_write_data(SCSIRequest *req)
         return;
     }
 
+    /* Save a ref for scsi_write_complete, in case r is canceled.  */
+    scsi_req_ref(&r->req);
     n = r->qiov.size / 512;
     if (n) {
         if (s->tray_open) {
@@ -343,6 +353,8 @@ static void scsi_dma_restart_bh(void *opaque)
                     scsi_req_complete(&r->req, GOOD);
                 }
             }
+            /* This reference was left in by scsi_handle_rw_error.  */
+            scsi_req_unref(&r->req);
         }
     }
 }
@@ -1324,6 +1336,8 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
         r->iov.iov_len = rc;
         break;
     case SYNCHRONIZE_CACHE:
+        /* Save a ref for scsi_flush_complete, in case r is canceled.  */
+        scsi_req_ref(&r->req);
         bdrv_acct_start(s->bs, &r->acct, 0, BDRV_ACCT_FLUSH);
         r->req.aiocb = bdrv_aio_flush(s->bs, scsi_flush_complete, r);
         if (r->req.aiocb == NULL) {
-- 
1.7.6

[Qemu-devel] [PATCH 4/4] scsi-disk: fix retrying a flush

[Paolo Bonzini]

Flush does not go anymore through scsi_disk_emulate_command.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index d6f2345..eb0c679 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -81,7 +81,7 @@ struct SCSIDiskState
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
-static int scsi_disk_emulate_command(SCSIDiskReq *r);
+static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf);
 
 static void scsi_free_request(SCSIRequest *req)
 {
@@ -335,7 +335,6 @@ static void scsi_dma_restart_bh(void *opaque)
         r = DO_UPCAST(SCSIDiskReq, req, req);
         if (r->status & SCSI_REQ_STATUS_RETRY) {
             int status = r->status;
-            int ret;
 
             r->status &=
                 ~(SCSI_REQ_STATUS_RETRY | SCSI_REQ_STATUS_RETRY_TYPE_MASK);
@@ -348,10 +347,8 @@ static void scsi_dma_restart_bh(void *opaque)
                 scsi_write_data(&r->req);
                 break;
             case SCSI_REQ_STATUS_RETRY_FLUSH:
-                ret = scsi_disk_emulate_command(r);
-                if (ret == 0) {
-                    scsi_req_complete(&r->req, GOOD);
-                }
+                scsi_send_command(&r->req, r->req.cmd.buf);
+                break;
             }
             /* This reference was left in by scsi_handle_rw_error.  */
             scsi_req_unref(&r->req);
-- 
1.7.6