Re: [Qemu-devel] [PATCH] scsi: check current request object before use

[Paolo Bonzini <pbonzini@redhat.com>]

On 06/12/2017 12:34, Darren Kenny wrote:
> Are both tests for NULL necessary, the second one would seem to
> suffice - but also the first check changes whether esp_dma_done()
> would get called or not here:
> 
>  276     if (s->async_len == 0) {
>  277         scsi_req_continue(s->current_req);
>  278         /* If there is still data to be read from the device then
>  279            complete the DMA operation immediately.  Otherwise defer
>  280            until the scsi layer has completed.  */
>  281         if (to_device || s->dma_left != 0 || s->ti_size == 0) {
>  282             return;
>  283         }
>  284     }
>  285  286     /* Partially filled a scsi buffer. Complete immediately.  */
>  287     esp_dma_done(s);
> 
> I don't know the SCSI code well enough to know if it is correct to
> change this, so just pointing it out in case someone else might.

The second test should definitely not be necessary, it's only papering
over a bug elsewhere.  But without a testcase it's not possible for me
to judge if the first test is the right fix, or _also_ papering over a
bug elsewhere.

Thanks,

Paolo

> Thanks,
> 
> Darren.
> 
> On Wed, Dec 06, 2017 at 04:51:16PM +0530, P J P wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> During a dma access, SCSIRequest object 'current_req' could be
>> null, leading to a null pointer dereference. Add check to avoid
>> it.
>>
>> Reported-by: Zhangboxian <zhangboxian@huawei.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>> hw/scsi/esp.c      | 2 +-
>> hw/scsi/scsi-bus.c | 3 +++
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index ee586e7d6c..0c6925a708 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -273,7 +273,7 @@ static void esp_do_dma(ESPState *s)
>>         s->ti_size += len;
>>     else
>>         s->ti_size -= len;
>> -    if (s->async_len == 0) {
>> +    if (s->current_req && s->async_len == 0) {
>>         scsi_req_continue(s->current_req);
>>         /* If there is still data to be read from the device then
>>            complete the DMA operation immediately.  Otherwise defer
>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>> index 977f7bce1f..ac64a239e9 100644
>> --- a/hw/scsi/scsi-bus.c
>> +++ b/hw/scsi/scsi-bus.c
>> @@ -1338,6 +1338,9 @@ void scsi_req_unref(SCSIRequest *req)
>>    will start the next chunk or complete the command.  */
>> void scsi_req_continue(SCSIRequest *req)
>> {
>> +    if (!req) {
>> +        return;
>> +    }
>>     if (req->io_canceled) {
>>         trace_scsi_req_continue_canceled(req->dev->id, req->lun,
>> req->tag);
>>         return;
>> -- 
>> 2.13.6
>>
>>