On 05/09/2016 22:50, P J P wrote: > From: Prasad J Pandit <p...@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. Check > SG list element count to avoid it.
The commit message is not correct, because you're fixing the bug in two different ways: by removing the cast and by limiting the number of iterations in the loop. --- In PVSCSI paravirtual SCSI bus, pvscsi_convert_sglist can take a very long time or go into an infinite loop due to two different bugs: 1) 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 large 'dataLen' values when data_length is cast to uint32_t and chunk_size becomes always zero. Fix this by removing the incorrect cast. 2) pvscsi_get_next_sg_elem can be called arbitrarily many times if the element has a zero length. Get out of the loop early when this happens, by introducing an upper limit on the number of SG list elements. --- Paolo > Reported-by: Li Qiang <liqiang...@360.cn> > Signed-off-by: Prasad J Pandit <p...@fedoraproject.org> > --- > hw/scsi/vmw_pvscsi.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > Update per: > -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg00594.html > > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c > index 4245c15..babac5a 100644 > --- a/hw/scsi/vmw_pvscsi.c > +++ b/hw/scsi/vmw_pvscsi.c > @@ -40,6 +40,8 @@ > #define PVSCSI_MAX_DEVS (64) > #define PVSCSI_MSIX_NUM_VECTORS (1) > > +#define PVSCSI_MAX_SG_ELEM 2048 > + > #define PVSCSI_MAX_CMD_DATA_WORDS \ > (sizeof(PVSCSICmdDescSetupRings)/sizeof(uint32_t)) > > @@ -628,17 +630,16 @@ pvscsi_queue_pending_descriptor(PVSCSIState *s, > SCSIDevice **d, > static void > pvscsi_convert_sglist(PVSCSIRequest *r) > { > - int chunk_size; > + uint32_t chunk_size, elmcnt = 0; > uint64_t data_length = r->req.dataLen; > PVSCSISGState sg = r->sg; > - while (data_length) { > - while (!sg.resid) { > + while (data_length && elmcnt < PVSCSI_MAX_SG_ELEM) { > + while (!sg.resid && elmcnt++ < PVSCSI_MAX_SG_ELEM) { > pvscsi_get_next_sg_elem(&sg); > 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); > } >
