From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965794AbcBDNMz (ORCPT ); Thu, 4 Feb 2016 08:12:55 -0500 Received: from mail-wm0-f53.google.com ([74.125.82.53]:38305 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964999AbcBDNMx (ORCPT ); Thu, 4 Feb 2016 08:12:53 -0500 Subject: Re: [PATCH] vfio/pci: Fix unsigned comparison overflow To: Alex Williamson References: <20160201235430.21791.64530.stgit@gimli.home> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org From: Eric Auger Message-ID: <56B34E3C.9010105@linaro.org> Date: Thu, 4 Feb 2016 14:12:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160201235430.21791.64530.stgit@gimli.home> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alex, Reviewed-by: Eric Auger Tested-by: Eric Auger Best Regards Eric On 02/02/2016 12:54 AM, Alex Williamson wrote: > Signed versus unsigned comparisons are implicitly cast to unsigned, > which result in a couple possible overflows. For instance (start + > count) might overflow and wrap, getting through our validation test. > Also when unwinding setup, -1 being compared as unsigned doesn't > produce the intended stop condition. Fix both of these and also fix > vfio_msi_set_vector_signal() to validate parameters before using the > vector index, though none of the callers should pass bad indexes > anymore. > > Reported-by: Eric Auger > Signed-off-by: Alex Williamson > --- > drivers/vfio/pci/vfio_pci_intrs.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 3b3ba15..e9ea3fe 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -309,14 +309,14 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > int vector, int fd, bool msix) > { > struct pci_dev *pdev = vdev->pdev; > - int irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; > - char *name = msix ? "vfio-msix" : "vfio-msi"; > struct eventfd_ctx *trigger; > - int ret; > + int irq, ret; > > - if (vector >= vdev->num_ctx) > + if (vector < 0 || vector >= vdev->num_ctx) > return -EINVAL; > > + irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; > + > if (vdev->ctx[vector].trigger) { > free_irq(irq, vdev->ctx[vector].trigger); > irq_bypass_unregister_producer(&vdev->ctx[vector].producer); > @@ -328,8 +328,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > if (fd < 0) > return 0; > > - vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "%s[%d](%s)", > - name, vector, pci_name(pdev)); > + vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "vfio-msi%s[%d](%s)", > + msix ? "x" : "", vector, > + pci_name(pdev)); > if (!vdev->ctx[vector].name) > return -ENOMEM; > > @@ -379,7 +380,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > { > int i, j, ret = 0; > > - if (start + count > vdev->num_ctx) > + if (start >= vdev->num_ctx || start + count > vdev->num_ctx) > return -EINVAL; > > for (i = 0, j = start; i < count && !ret; i++, j++) { > @@ -388,7 +389,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > } > > if (ret) { > - for (--j; j >= start; j--) > + for (--j; j >= (int)start; j--) > vfio_msi_set_vector_signal(vdev, j, -1, msix); > } > >