scsi: check current request object before use

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

Posted by P J P 6 years, 4 months ago

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

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

Posted by Paolo Bonzini 6 years, 4 months ago

On 06/12/2017 12:21, 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>

Sorry, this needs a test case.

Paolo

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

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

Posted by Darren Kenny 6 years, 4 months ago

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.

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

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

Posted by Paolo Bonzini 6 years, 4 months ago

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