Patchwork [v3,13/21] scsi: do not call send_command directly

login
register
mail settings
Submitter Paolo Bonzini
Date May 17, 2011, 11 a.m.
Message ID <1305630067-2119-14-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/95912/
State New
Headers show

Comments

Paolo Bonzini - May 17, 2011, 11 a.m.
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(-)
Christoph Hellwig - May 20, 2011, 4:04 p.m.
> -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;

How would it disappear given that we grabbed another reference just before?
That probably needs a bit more documentation here.  Also why not move
the two scsi_req_ref calls together?
Paolo Bonzini - May 20, 2011, 5:43 p.m.
On 05/20/2011 06:04 PM, Christoph Hellwig wrote:
>> -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;
>
> How would it disappear given that we grabbed another reference just before?

Welcome to the wonderful world of nested callbacks. :(

Suppose send_command completes a request.  scsi_req_complete then 
dequeues it (which undoes the reference above), and calls the device who 
owned it.  The device who owned the request then presumably NULLs a 
pointer and drops the last reference.  The request is then freed.

This was exactly the kind of scenario that I was worried about when I 
thought about reference counting.  I couldn't actually put my fingers on 
it, but I knew it was broken somewhere, and indeed a use-after-free was 
reported less than a month later.

It is not strictly necessary to wrap send_command with a ref/unref pair, 
but it is quite convenient when writing send_command.  It also mirrors 
what I do around scsi_req_complete and scsi_req_cancel.

> That probably needs a bit more documentation here.  Also why not move
> the two scsi_req_ref calls together?

Because one is logically related to putting the request in the queue, 
while the other is related to the send_command op.

Paolo
Kevin Wolf - May 24, 2011, 1:05 p.m.
Am 20.05.2011 19:43, schrieb Paolo Bonzini:
> On 05/20/2011 06:04 PM, Christoph Hellwig wrote:
>>> -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;
>>
>> How would it disappear given that we grabbed another reference just before?
> 
> Welcome to the wonderful world of nested callbacks. :(
> 
> Suppose send_command completes a request.  scsi_req_complete then 
> dequeues it (which undoes the reference above), and calls the device who 
> owned it.  The device who owned the request then presumably NULLs a 
> pointer and drops the last reference.  The request is then freed.

Maybe the callback should be done from a BH then? It sounds like this
could cause more bugs than what you're fixing here.

Kevin
Paolo Bonzini - May 24, 2011, 1:13 p.m.
On 05/24/2011 03:05 PM, Kevin Wolf wrote:
> Maybe the callback should be done from a BH then? It sounds like this
> could cause more bugs than what you're fixing here.

Not sure, after all it makes sense to answer some queries synchronously 
(e.g. TEST_UNIT_READY).  It's just the convoluted control flow that 
tricked you when you moved accesses to after the request has been 
completed.  With reference counting, the data in the SCSIRequest remains 
completely valid after it has been completed and until the last ref goes 
away, so I see no reason to complicate the logic further by introducing 
a BH.

Paolo

Patch

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) {