Re: [Qemu-devel] [PATCH v3] scsi: pvscsi: avoid infinite loop while building SG list

[Dmitry Fleytman <dmitry@daynix.com>]

Hello Prasad,

See my comments inline.

> On 13 Sep 2016, at 10:01 AM, P J P <ppandit@redhat.com> wrote:
> 
> +-- On Tue, 6 Sep 2016, P J P wrote --+
> | From: Prasad J Pandit <pjp@fedoraproject.org>
> | 
> | 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 for 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.
> | 
> | 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 as per:
> |   -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01172.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) {


Better put here:

if(elemcnt++ >= PVSCSI_MAX_SG_ELEM) {
    return;
}

And ditch additional conditions from while clauses.
This way you’ll have less compare operations and avoid
adding the same element twice when leaving because of too long SG list.

Also this is not a good idea to send truncated requests
when SG list becomes too long. I’d prefer to return error
code from pvscsi_convert_sglist() and handle it as appropriate.


Regards,
Dmitry

> |              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);
> |          }
> 
> 
> Ping...!
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F