| Message ID | 1473108643-12983-1-git-send-email-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/09/2016 22:50, 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. 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 <liqiang6-s@360.cn> > Signed-off-by: Prasad J Pandit <pjp@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); > } >
Hello Paolo, +-- On Tue, 6 Sep 2016, Paolo Bonzini wrote --+ | 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. Ah, yes; Sorry about that. | --- | 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. | --- Done; I've sent a revised patch v3 with this commit message. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
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); }