[Qemu-devel] [PATCH v2] scsi: pvscsi: check request descriptor SG element count

[Qemu-devel] [PATCH v2] scsi: pvscsi: check request descriptor SG element count

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

In PVSCSI paravirtual SCSI bus, 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 arbitrarily large 'dataLen' values. Check
SG list element count to avoid  it.

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 per:
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00594.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);
         }
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2] scsi: pvscsi: check request descriptor SG element count

[Paolo Bonzini]

On 05/09/2016 22:50, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> In PVSCSI paravirtual SCSI bus, 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 arbitrarily large 'dataLen' values. Check
> SG list element count to avoid  it.

The commit message is not correct, because you're fixing the bug in two
different ways: by removing the cast and by limiting the number of
iterations in the loop.

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

Paolo

> 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 per:
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00594.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);
>          }
> 

Re: [Qemu-devel] [PATCH v2] scsi: pvscsi: check request descriptor SG element count

[P J P]

  Hello Paolo,

+-- On Tue, 6 Sep 2016, Paolo Bonzini wrote --+
| The commit message is not correct, because you're fixing the bug in two
| different ways: by removing the cast and by limiting the number of
| iterations in the loop.

  Ah, yes; Sorry about that.

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

  Done; I've sent a revised patch v3 with this commit message.

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