Re: [Qemu-devel] [PATCH] scsi: pvscsi: request descriptor data_length to 32 bit

[Paolo Bonzini]

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