| Message ID | 1472884428-9975-1-git-send-email-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; 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); }