diff mbox

[v3] scsi: pvscsi: avoid infinite loop while building SG list

Message ID 1473184397-27900-1-git-send-email-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Prasad Pandit Sept. 6, 2016, 5:53 p.m. UTC 
From: Prasad J Pandit <pjp@fedoraproject.org>

In PVSCSI paravirtual SCSI bus, pvscsi_convert_sglist can take a very
long time or go into an infinite loop due to two different bugs:

1) the request descriptor data length is defined to be 64 bit. While
building SG list from a request descriptor, it gets truncated to 32bit
in routine 'pvscsi_convert_sglist'. This could lead to an infinite loop
situation for large 'dataLen' values, when data_length is cast to uint32_t
and chunk_size becomes always zero.  Fix this by removing the incorrect
cast.

2) pvscsi_get_next_sg_elem can be called arbitrarily many times if the
element has a zero length.  Get out of the loop early when this happens,
by introducing an upper limit on the number of SG list elements.

Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/scsi/vmw_pvscsi.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Update as per:
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01172.html

Comments

Prasad Pandit Sept. 13, 2016, 7:01 a.m. UTC | #1 
+-- On Tue, 6 Sep 2016, P J P wrote --+
| From: Prasad J Pandit <pjp@fedoraproject.org>
| 
| In PVSCSI paravirtual SCSI bus, pvscsi_convert_sglist can take a very
| long time or go into an infinite loop due to two different bugs:
| 
| 1) the request descriptor data length is defined to be 64 bit. While
| building SG list from a request descriptor, it gets truncated to 32bit
| in routine 'pvscsi_convert_sglist'. This could lead to an infinite loop
| situation for large 'dataLen' values, when data_length is cast to uint32_t
| and chunk_size becomes always zero.  Fix this by removing the incorrect
| cast.
| 
| 2) pvscsi_get_next_sg_elem can be called arbitrarily many times if the
| element has a zero length.  Get out of the loop early when this happens,
| by introducing an upper limit on the number of SG list elements.
| 
| Reported-by: Li Qiang <liqiang6-s@360.cn>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
|  hw/scsi/vmw_pvscsi.c | 11 ++++++-----
|  1 file changed, 6 insertions(+), 5 deletions(-)
| 
| Update as per:
|   -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01172.html
| 
| diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
| index 4245c15..babac5a 100644
| --- a/hw/scsi/vmw_pvscsi.c
| +++ b/hw/scsi/vmw_pvscsi.c
| @@ -40,6 +40,8 @@
|  #define PVSCSI_MAX_DEVS                   (64)
|  #define PVSCSI_MSIX_NUM_VECTORS           (1)
|  
| +#define PVSCSI_MAX_SG_ELEM                2048
| +
|  #define PVSCSI_MAX_CMD_DATA_WORDS \
|      (sizeof(PVSCSICmdDescSetupRings)/sizeof(uint32_t))
|  
| @@ -628,17 +630,16 @@ pvscsi_queue_pending_descriptor(PVSCSIState *s, SCSIDevice **d,
|  static void
|  pvscsi_convert_sglist(PVSCSIRequest *r)
|  {
| -    int chunk_size;
| +    uint32_t chunk_size, elmcnt = 0;
|      uint64_t data_length = r->req.dataLen;
|      PVSCSISGState sg = r->sg;
| -    while (data_length) {
| -        while (!sg.resid) {
| +    while (data_length && elmcnt < PVSCSI_MAX_SG_ELEM) {
| +        while (!sg.resid && elmcnt++ < PVSCSI_MAX_SG_ELEM) {
|              pvscsi_get_next_sg_elem(&sg);
|              trace_pvscsi_convert_sglist(r->req.context, r->sg.dataAddr,
|                                          r->sg.resid);
|          }
| -        assert(data_length > 0);
| -        chunk_size = MIN((unsigned) data_length, sg.resid);
| +        chunk_size = MIN(data_length, sg.resid);
|          if (chunk_size) {
|              qemu_sglist_add(&r->sgl, sg.dataAddr, chunk_size);
|          }


Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Dmitry Fleytman Sept. 13, 2016, 8:54 a.m. UTC | #2 
Hello Prasad,

See my comments inline.

> On 13 Sep 2016, at 10:01 AM, P J P <ppandit@redhat.com> wrote:
> 
> +-- On Tue, 6 Sep 2016, P J P wrote --+
> | From: Prasad J Pandit <pjp@fedoraproject.org>
> | 
> | In PVSCSI paravirtual SCSI bus, pvscsi_convert_sglist can take a very
> | long time or go into an infinite loop due to two different bugs:
> | 
> | 1) the request descriptor data length is defined to be 64 bit. While
> | building SG list from a request descriptor, it gets truncated to 32bit
> | in routine 'pvscsi_convert_sglist'. This could lead to an infinite loop
> | situation for large 'dataLen' values, when data_length is cast to uint32_t
> | and chunk_size becomes always zero.  Fix this by removing the incorrect
> | cast.
> | 
> | 2) pvscsi_get_next_sg_elem can be called arbitrarily many times if the
> | element has a zero length.  Get out of the loop early when this happens,
> | by introducing an upper limit on the number of SG list elements.
> | 
> | Reported-by: Li Qiang <liqiang6-s@360.cn>
> | Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> | ---
> |  hw/scsi/vmw_pvscsi.c | 11 ++++++-----
> |  1 file changed, 6 insertions(+), 5 deletions(-)
> | 
> | Update as per:
> |   -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01172.html
> | 
> | diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> | index 4245c15..babac5a 100644
> | --- a/hw/scsi/vmw_pvscsi.c
> | +++ b/hw/scsi/vmw_pvscsi.c
> | @@ -40,6 +40,8 @@
> |  #define PVSCSI_MAX_DEVS                   (64)
> |  #define PVSCSI_MSIX_NUM_VECTORS           (1)
> |  
> | +#define PVSCSI_MAX_SG_ELEM                2048
> | +
> |  #define PVSCSI_MAX_CMD_DATA_WORDS \
> |      (sizeof(PVSCSICmdDescSetupRings)/sizeof(uint32_t))
> |  
> | @@ -628,17 +630,16 @@ pvscsi_queue_pending_descriptor(PVSCSIState *s, SCSIDevice **d,
> |  static void
> |  pvscsi_convert_sglist(PVSCSIRequest *r)
> |  {
> | -    int chunk_size;
> | +    uint32_t chunk_size, elmcnt = 0;
> |      uint64_t data_length = r->req.dataLen;
> |      PVSCSISGState sg = r->sg;
> | -    while (data_length) {
> | -        while (!sg.resid) {
> | +    while (data_length && elmcnt < PVSCSI_MAX_SG_ELEM) {
> | +        while (!sg.resid && elmcnt++ < PVSCSI_MAX_SG_ELEM) {


Better put here:

if(elemcnt++ >= PVSCSI_MAX_SG_ELEM) {
    return;
}

And ditch additional conditions from while clauses.
This way you’ll have less compare operations and avoid
adding the same element twice when leaving because of too long SG list.

Also this is not a good idea to send truncated requests
when SG list becomes too long. I’d prefer to return error
code from pvscsi_convert_sglist() and handle it as appropriate.


Regards,
Dmitry

> |              pvscsi_get_next_sg_elem(&sg);
> |              trace_pvscsi_convert_sglist(r->req.context, r->sg.dataAddr,
> |                                          r->sg.resid);
> |          }
> | -        assert(data_length > 0);
> | -        chunk_size = MIN((unsigned) data_length, sg.resid);
> | +        chunk_size = MIN(data_length, sg.resid);
> |          if (chunk_size) {
> |              qemu_sglist_add(&r->sgl, sg.dataAddr, chunk_size);
> |          }
> 
> 
> Ping...!
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Prasad Pandit Sept. 13, 2016, 11:20 a.m. UTC | #3 
Hello Dmitry,

+-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
| Better put here:
| 
| if(elemcnt++ >= PVSCSI_MAX_SG_ELEM) {
|     return;
| }
|
| And ditch additional conditions from while clauses.
| This way you’ll have less compare operations and avoid
| adding the same element twice when leaving because of too long SG list.

  Okay.

| Also this is not a good idea to send truncated requests
| when SG list becomes too long. I’d prefer to return error
| code from pvscsi_convert_sglist() and handle it as appropriate.

  This could be a separate patch. Should pvscsi_build_sglist() destroy the 
sglist on error? 

@@ -656,7 +660,9 @@ pvscsi_build_sglist(PVSCSIState *s, PVSCSIRequest *r)
 
     pci_dma_sglist_init(&r->sgl, d, 1);
     if (r->req.flags & PVSCSI_FLAG_CMD_WITH_SG_LIST) {
-        pvscsi_convert_sglist(r);
+        if (pvscsi_convert_sglist(r) < 0) {
+            qemu_sglist_destroy(&r->sgl);
+        }
     } else {
         qemu_sglist_add(&r->sgl, r->req.dataAddr, r->req.dataLen);
     }

I'll send both patches together once you confirm.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Dmitry Fleytman Sept. 13, 2016, noon UTC | #4 
> On 13 Sep 2016, at 14:20 PM, P J P <ppandit@redhat.com> wrote:
> 
>  Hello Dmitry,
> 
> +-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
> | Better put here:
> | 
> | if(elemcnt++ >= PVSCSI_MAX_SG_ELEM) {
> |     return;
> | }
> |
> | And ditch additional conditions from while clauses.
> | This way you’ll have less compare operations and avoid
> | adding the same element twice when leaving because of too long SG list.
> 
>  Okay.
> 
> | Also this is not a good idea to send truncated requests
> | when SG list becomes too long. I’d prefer to return error
> | code from pvscsi_convert_sglist() and handle it as appropriate.
> 
>  This could be a separate patch. Should pvscsi_build_sglist() destroy the 
> sglist on error? 

It should be in the same patch because otherwise a broken logic will be introduced.

pvscsi_build_sglist() must perform all rollback actions required to handle the error cleanly.
Please also note that pvscsi_build_sglist() caller(s) should be aware of SG list creation failure
and handle this error scenario accordingly.

> 
> @@ -656,7 +660,9 @@ pvscsi_build_sglist(PVSCSIState *s, PVSCSIRequest *r)
> 
>     pci_dma_sglist_init(&r->sgl, d, 1);
>     if (r->req.flags & PVSCSI_FLAG_CMD_WITH_SG_LIST) {
> -        pvscsi_convert_sglist(r);
> +        if (pvscsi_convert_sglist(r) < 0) {
> +            qemu_sglist_destroy(&r->sgl);
> +        }
>     } else {
>         qemu_sglist_add(&r->sgl, r->req.dataAddr, r->req.dataLen);
>     }
> 
> I'll send both patches together once you confirm.
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Prasad Pandit Sept. 13, 2016, 12:53 p.m. UTC | #5 
Hello Dmitry,

+-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
| It should be in the same patch because otherwise a broken logic will be 
| introduced.

  Not even same patch-set?

| pvscsi_build_sglist() must perform all rollback actions required to handle 
| the error cleanly. Please also note that pvscsi_build_sglist() caller(s) 
| should be aware of SG list creation failure and handle this error scenario 
| accordingly.

  I see. All callers leading up to 'pvscsi_convert_sglist' appear to be 
returning void.

MemoryRegionOps pvscsi_ops = {
.write = pvscsi_io_write
 -> pvscsi_io_write
  -> pvscsi_process_io
   ->pvscsi_process_request_descriptor
    -> pvscsi_build_sglist
     -> pvscsi_convert_sglist

This looks to be a big patch to fix an infinite loop issue. It'll help to 
separate the two patches, no?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Dmitry Fleytman Sept. 13, 2016, 1:05 p.m. UTC | #6 
> On 13 Sep 2016, at 15:53 PM, P J P <ppandit@redhat.com> wrote:
> 
>  Hello Dmitry,
> 
> +-- On Tue, 13 Sep 2016, Dmitry Fleytman wrote --+
> | It should be in the same patch because otherwise a broken logic will be 
> | introduced.
> 
>  Not even same patch-set?

Same patch-set might be good as well.

> 
> | pvscsi_build_sglist() must perform all rollback actions required to handle 
> | the error cleanly. Please also note that pvscsi_build_sglist() caller(s) 
> | should be aware of SG list creation failure and handle this error scenario 
> | accordingly.
> 
>  I see. All callers leading up to 'pvscsi_convert_sglist' appear to be 
> returning void.
> 
> MemoryRegionOps pvscsi_ops = {
> .write = pvscsi_io_write
> -> pvscsi_io_write
>  -> pvscsi_process_io
>   ->pvscsi_process_request_descriptor
>    -> pvscsi_build_sglist
>     -> pvscsi_convert_sglist
> 
> This looks to be a big patch to fix an infinite loop issue. It'll help to 
> separate the two patches, no?

Maybe. Let’s see the patches.

> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff mbox

Patch

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index 4245c15..babac5a 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -40,6 +40,8 @@ 
 #define PVSCSI_MAX_DEVS                   (64)
 #define PVSCSI_MSIX_NUM_VECTORS           (1)
 
+#define PVSCSI_MAX_SG_ELEM                2048
+
 #define PVSCSI_MAX_CMD_DATA_WORDS \
     (sizeof(PVSCSICmdDescSetupRings)/sizeof(uint32_t))
 
@@ -628,17 +630,16 @@  pvscsi_queue_pending_descriptor(PVSCSIState *s, SCSIDevice **d,
 static void
 pvscsi_convert_sglist(PVSCSIRequest *r)
 {
-    int chunk_size;
+    uint32_t chunk_size, elmcnt = 0;
     uint64_t data_length = r->req.dataLen;
     PVSCSISGState sg = r->sg;
-    while (data_length) {
-        while (!sg.resid) {
+    while (data_length && elmcnt < PVSCSI_MAX_SG_ELEM) {
+        while (!sg.resid && elmcnt++ < PVSCSI_MAX_SG_ELEM) {
             pvscsi_get_next_sg_elem(&sg);
             trace_pvscsi_convert_sglist(r->req.context, r->sg.dataAddr,
                                         r->sg.resid);
         }
-        assert(data_length > 0);
-        chunk_size = MIN((unsigned) data_length, sg.resid);
+        chunk_size = MIN(data_length, sg.resid);
         if (chunk_size) {
             qemu_sglist_add(&r->sgl, sg.dataAddr, chunk_size);
         }