From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhHJA-0003LM-Il for qemu-devel@nongnu.org; Tue, 06 Sep 2016 10:25:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhHJ5-0005h7-Eg for qemu-devel@nongnu.org; Tue, 06 Sep 2016 10:25:03 -0400 Received: from mail-wm0-x244.google.com ([2a00:1450:400c:c09::244]:36797) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhHJ5-0005gq-4W for qemu-devel@nongnu.org; Tue, 06 Sep 2016 10:24:59 -0400 Received: by mail-wm0-x244.google.com with SMTP id l65so11015538wmf.3 for ; Tue, 06 Sep 2016 07:24:59 -0700 (PDT) Sender: Paolo Bonzini References: <1473108643-12983-1-git-send-email-ppandit@redhat.com> From: Paolo Bonzini Message-ID: Date: Tue, 6 Sep 2016 16:24:56 +0200 MIME-Version: 1.0 In-Reply-To: <1473108643-12983-1-git-send-email-ppandit@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2] scsi: pvscsi: check request descriptor SG element count List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P , Qemu Developers Cc: Dmitry Fleytman , Li Qiang , Prasad J Pandit On 05/09/2016 22:50, P J P wrote: > From: Prasad J Pandit > > 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 > Signed-off-by: Prasad J Pandit > --- > 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); > } >