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

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

> -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?

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

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

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