[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] scsi: pvscsi: check request descriptor SG el
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2] scsi: pvscsi: check request descriptor SG element count |
Date: |
Tue, 6 Sep 2016 16:24:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
On 05/09/2016 22:50, P J P wrote:
> From: Prasad J Pandit <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
> 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);
> }
>