[Qemu-devel] [PATCH v3 13/21] scsi: do not call send_command directly

[Paolo Bonzini <pbonzini@redhat.com>]

Move the common part of scsi-disk.c and scsi-generic.c to the SCSI layer.
At the same time, protect against the request being freed under the
feet of the send_command callback.

This fixes a use-after-free that happened when scsi-disk's
scsi_disk_emulate_command completed an illegal request, and still its
caller scsi_send_command accessed r->sector_count and r->iov.iov_len.
The return value from scsi_send_command was then bogus; the HBA device
model mistook the completed request for an I/O request and typically
SIGSEGVed on a NULL pointer access to the current request.

Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Tested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/esp.c          |    2 +-
 hw/lsi53c895a.c   |    2 +-
 hw/scsi-bus.c     |    9 ++++++++-
 hw/scsi-disk.c    |    1 -
 hw/scsi-generic.c |    1 -
 hw/scsi.h         |    2 +-
 hw/spapr_vscsi.c  |    4 ++--
 hw/usb-msd.c      |    2 +-
 8 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/hw/esp.c b/hw/esp.c
index 46157a8..3a65aed 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -245,7 +245,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
      DPRINTF("do_busid_cmd: busid 0x%x\n", busid);
     lun = busid & 7;
     s->current_req = s->current_dev->info->alloc_req(s->current_dev, 0, lun);
-    datalen = s->current_dev->info->send_command(s->current_req, buf);
+    datalen = scsi_req_enqueue(s->current_req, buf);
     s->ti_size = datalen;
     if (datalen != 0) {
         s->rregs[ESP_RSTAT] = STAT_TC;
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f78b85f..b2dbcaa 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -791,7 +791,7 @@ static void lsi_do_command(LSIState *s)
     s->current->req = dev->info->alloc_req(dev, s->current->tag,
                                            s->current_lun);
 
-    n = dev->info->send_command(s->current->req, buf);
+    n = scsi_req_enqueue(s->current->req, buf);
     if (n > 0) {
         lsi_set_phase(s, PHASE_DI);
         dev->info->read_data(s->current->req);
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index b83dd88..0b54a4c 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -146,12 +146,19 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l
     return req;
 }
 
-void scsi_req_enqueue(SCSIRequest *req)
+int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf)
 {
+    int32_t rc;
     assert(!req->enqueued);
     scsi_req_ref(req);
     req->enqueued = true;
     QTAILQ_INSERT_TAIL(&req->dev->requests, req, next);
+
+    /* Make sure the request doesn't disappear under send_command's feet.  */
+    scsi_req_ref(req);
+    rc = req->dev->info->send_command(req, buf);
+    scsi_req_unref(req);
+    return rc;
 }
 
 static void scsi_req_dequeue(SCSIRequest *req)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a82753f..efb953b 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -982,7 +982,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf)
     uint8_t *outbuf;
     int rc;
 
-    scsi_req_enqueue(req);
     command = buf[0];
     outbuf = (uint8_t *)r->iov.iov_base;
     is_write = 0;
diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index b934ba4..036ab9f 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -318,7 +318,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
     SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
     int ret;
 
-    scsi_req_enqueue(req);
     if (cmd[0] != REQUEST_SENSE &&
         (req->lun != s->lun || (cmd[1] >> 5) != s->lun)) {
         DPRINTF("Unimplemented LUN %d\n", req->lun ? req->lun : cmd[1] >> 5);
diff --git a/hw/scsi.h b/hw/scsi.h
index 61ab7c9..c36c5cc 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -143,7 +143,7 @@ int scsi_build_sense(SCSISense sense, uint8_t *buf, int len, int fixed);
 int scsi_sense_valid(SCSISense sense);
 
 SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun);
-void scsi_req_enqueue(SCSIRequest *req);
+int32_t scsi_req_enqueue(SCSIRequest *req, uint8_t *buf);
 void scsi_req_free(SCSIRequest *req);
 SCSIRequest *scsi_req_ref(SCSIRequest *req);
 void scsi_req_unref(SCSIRequest *req);
diff --git a/hw/spapr_vscsi.c b/hw/spapr_vscsi.c
index 5b97a83..307a17f 100644
--- a/hw/spapr_vscsi.c
+++ b/hw/spapr_vscsi.c
@@ -459,7 +459,7 @@ static void vscsi_send_request_sense(VSCSIState *s, vscsi_req *req)
     cdb[4] = 96;
     cdb[5] = 0;
     req->sensing = 1;
-    n = sdev->info->send_command(req->sreq, cdb);
+    n = scsi_req_enqueue(req->sreq, cdb);
     dprintf("VSCSI: Queued request sense tag 0x%x\n", req->qtag);
     if (n < 0) {
         fprintf(stderr, "VSCSI: REQUEST_SENSE wants write data !?!?!?\n");
@@ -654,7 +654,7 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
     req->sdev = sdev;
     req->lun = lun;
     req->sreq = sdev->info->alloc_req(sdev, req->qtag, lun);
-    n = sdev->info->send_command(req->sreq, srp->cmd.cdb);
+    n = scsi_req_enqueue(req->sreq, srp->cmd.cdb);
 
     dprintf("VSCSI: Queued command tag 0x%x CMD 0x%x ID %d LUN %d ret: %d\n",
             req->qtag, srp->cmd.cdb[0], id, lun, n);
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 9b238e8..1375e82 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -378,7 +378,7 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
             s->residue = 0;
             s->scsi_len = 0;
             s->req = s->scsi_dev->info->alloc_req(s->scsi_dev, s->tag, 0);
-            s->scsi_dev->info->send_command(s->req, cbw.cmd);
+            scsi_req_enqueue(s->req, cbw.cmd);
             /* ??? Should check that USB and SCSI data transfer
                directions match.  */
             if (s->residue == 0) {
-- 
1.7.4.4