scsi: pvscsi: request descriptor data_length to 32 bit

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. Define local
variable 'data_length' to be 32 bit, 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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

On 03/09/2016 08:33, 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. Define local
> variable 'data_length' to be 32 bit, 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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 4245c15..4d38330 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -629,7 +629,7 @@ static void
>  pvscsi_convert_sglist(PVSCSIRequest *r)
>  {
>      int chunk_size;
> -    uint64_t data_length = r->req.dataLen;
> +    uint32_t data_length = r->req.dataLen;

Why is this needed if you remove the cast in MIN, below?

Paolo

>      PVSCSISGState sg = r->sg;
>      while (data_length) {
>          while (!sg.resid) {
> @@ -637,8 +637,7 @@ pvscsi_convert_sglist(PVSCSIRequest *r)
>              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);
>          }
>

Hello Paolo, all

+-- On Mon, 5 Sep 2016, Paolo Bonzini wrote --+
| > -    uint64_t data_length = r->req.dataLen;
| > +    uint32_t data_length = r->req.dataLen;
| 
| Why is this needed if you remove the cast in MIN, below?

The outer while loop below is controlled by 'data_length'. The cast in MIN 
truncates a large(64bit) value of 'data_length' to zero(0), thus setting 
'chunk_size' to zero, which results in infinite loop as 'data_length' remains 
unchanged(> 0).

Second, Removing cast below results in 'chunk_size' being set to 'sg.resid', 
for large(>32bit) values of 'data_length'. Which results in an infinite loop 
because the inner 'while(!sg.resid)' loop takes forever to read non-zero 
values into 'sg.resid'.

Above type change ensures that outer while loop is not entered if 
'data_length' is zero. And removing cast ensures that inner while(!sg.resid) 
loop does not have run forever, ie. till large(64 bit) 'data_length' becomes 
zero.

Looking at the 'vmw_pvscsi.c' Linux kernel driver, 'dataLen' seems to be set 
to an unsigned 32 bit 'bufflen' value.


| >      PVSCSISGState sg = r->sg;
| >      while (data_length) {
| >          while (!sg.resid) {
| > @@ -637,8 +637,7 @@ pvscsi_convert_sglist(PVSCSIRequest *r)
| >              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);
| >          }
| > 
| 


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

On 05/09/2016 11:50, P J P wrote:
>   Hello Paolo, all
> 
> +-- On Mon, 5 Sep 2016, Paolo Bonzini wrote --+
> | > -    uint64_t data_length = r->req.dataLen;
> | > +    uint32_t data_length = r->req.dataLen;
> | 
> | Why is this needed if you remove the cast in MIN, below?
> 
> The outer while loop below is controlled by 'data_length'. The cast in MIN 
> truncates a large(64bit) value of 'data_length' to zero(0), thus setting 
> 'chunk_size' to zero, which results in infinite loop as 'data_length' remains 
> unchanged(> 0).
> 
> Second, Removing cast below results in 'chunk_size' being set to 'sg.resid', 
> for large(>32bit) values of 'data_length'. Which results in an infinite loop 
> because the inner 'while(!sg.resid)' loop takes forever to read non-zero 
> values into 'sg.resid'.

No, that's not what happens.  chunk_size is set to sg.resid, after which:

        sg.dataAddr += chunk_size;
        data_length -= chunk_size;
        sg.resid -= chunk_size;

The loop is reentered with sg.resid == 0, it calls into
pvscsi_get_next_sg_elem and this sets sg.resid to a nonzero value.  It's
not an infinite loop.

> Above type change ensures that outer while loop is not entered if 
> 'data_length' is zero. And removing cast ensures that inner while(!sg.resid) 
> loop does not have run forever, ie. till large(64 bit) 'data_length' becomes 
> zero.
> 
> Looking at the 'vmw_pvscsi.c' Linux kernel driver, 'dataLen' seems to be set 
> to an unsigned 32 bit 'bufflen' value.

The driver is irrelevant.  If the data_length is an uint64_t you need to
ensure that a 64 bit buffer is processed correctly.  Here you are
truncating it, which is wrong and will cause a buffer underrun.

Paolo

+-- On Mon, 5 Sep 2016, Paolo Bonzini wrote --+
| No, that's not what happens.  chunk_size is set to sg.resid, after which:
| 
|         sg.dataAddr += chunk_size;
|         data_length -= chunk_size;
|         sg.resid -= chunk_size;
| 
| The loop is reentered with sg.resid == 0, it calls into
| pvscsi_get_next_sg_elem and this sets sg.resid to a nonzero value.  It's
| not an infinite loop.

  Yes, true; But 'pvscsi_get_next_sg_elem' does not return non-zero 'sg.resid' 
each time. In fact, it returns more zeros and thus the loop iterates 
infinitely. When I ran it with 64 bit 'data_length' and without cast, after 
some time, the inner loop gets stuck and does not seem to read non-zero values 
into 'sg.resid'.

Is there limit to number of SG elements?

| The driver is irrelevant.  If the data_length is an uint64_t you need to
| ensure that a 64 bit buffer is processed correctly.  Here you are
| truncating it, which is wrong and will cause a buffer underrun.

  Yes. I thought truncation in MIN was intentional, considering the driver 
sets 'dataLen' to 32 bit value.

If we are to go with 64 bit 'data_length', how long should the inner while 
loop run?

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

On 05/09/2016 13:13, P J P wrote:
> +-- On Mon, 5 Sep 2016, Paolo Bonzini wrote --+
> | No, that's not what happens.  chunk_size is set to sg.resid, after which:
> | 
> |         sg.dataAddr += chunk_size;
> |         data_length -= chunk_size;
> |         sg.resid -= chunk_size;
> | 
> | The loop is reentered with sg.resid == 0, it calls into
> | pvscsi_get_next_sg_elem and this sets sg.resid to a nonzero value.  It's
> | not an infinite loop.
> 
>   Yes, true; But 'pvscsi_get_next_sg_elem' does not return non-zero 'sg.resid' 
> each time. In fact, it returns more zeros and thus the loop iterates 
> infinitely. When I ran it with 64 bit 'data_length' and without cast, after 
> some time, the inner loop gets stuck and does not seem to read non-zero values 
> into 'sg.resid'.

pvscsi_get_next_sg_elem just reads 16 bytes from guest RAM, so I guess
that's because you didn't set up the SG list correctly.  QEMU indeed
doesn't check for that, but that's a different bug.

> Is there limit to number of SG elements?

Without a public spec it's hard, but I guess 2048 is more than enough.

Paolo