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
