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

[P J P <ppandit@redhat.com>]

  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