From patchwork Fri Jan 29 21:41:16 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 8167701 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 3DD759F96D for ; Fri, 29 Jan 2016 21:43:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5E0E220381 for ; Fri, 29 Jan 2016 21:43:17 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7382B20374 for ; Fri, 29 Jan 2016 21:43:16 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aPGnV-0007SF-7i; Fri, 29 Jan 2016 21:41:41 +0000 Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1aPGnS-0007P1-Qe for linux-arm-kernel@lists.infradead.org; Fri, 29 Jan 2016 21:41:39 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id A709DC0B66C9; Fri, 29 Jan 2016 21:41:17 +0000 (UTC) Received: from t450s.home (ovpn-113-215.phx2.redhat.com [10.3.113.215]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0TLfHud030134; Fri, 29 Jan 2016 16:41:17 -0500 Message-ID: <1454103676.9301.3.camel@redhat.com> Subject: Re: [PATCH] vfio: pci: fix oops in case of vfio_msi_set_vector_signal failure From: Alex Williamson To: Eric Auger , eric.auger@st.com, linux-arm-kernel@lists.infradead.org, christoffer.dall@linaro.org Date: Fri, 29 Jan 2016 14:41:16 -0700 In-Reply-To: <1454078586-5431-1-git-send-email-eric.auger@linaro.org> References: <1454078586-5431-1-git-send-email-eric.auger@linaro.org> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160129_134138_934321_BAAB293B X-CRM114-Status: GOOD ( 22.05 ) X-Spam-Score: -6.9 (------) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-kernel@vger.kernel.org, patches@linaro.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, 2016-01-29 at 14:43 +0000, Eric Auger wrote: > In case vfio_msi_set_vector_signal fails we tear down everything. > In the tear down loop we compare int j against unsigned start. Given > the arithmetic conversion I think it is converted into an unsigned and > becomes 0xffffffff, leading to the loop being entered again and things > turn bad when accessing vdev->msix[vector].vector. So let's use int > parameters instead. >  > Signed-off-by: Eric Auger > --- >  drivers/vfio/pci/vfio_pci_intrs.c | 4 ++-- >  1 file changed, 2 insertions(+), 2 deletions(-) >  > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 3b3ba15..510c48d 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -374,8 +374,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, >   return 0; >  } >   > -static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > -       unsigned count, int32_t *fds, bool msix) > +static int vfio_msi_set_block(struct vfio_pci_device *vdev, int start, > +       int count, int32_t *fds, bool msix) >  { >   int i, j, ret = 0; >   Nice find, I don't think that's the only bug there though.  If @start is -1 (UINT32_MAX) and @count is 1, then @j gets set to -1 in the setup and we hit the same index dereference problem.  What if we did this instead: So we fix the problem with vfio_msi_set_vector_signal() dereferencing the array before it validates the index (even though it shouldn't be able to get there anymore), and then we do a better job of verifying start and count (comparing to num_ctx will use unsigned even though num_ctx itself is signed) and finally explicitly test the <0 case, which I suppose we could also do by casting start at that point (we know it's within the bounds of a signed integer given the previous tests). Thanks, Alex diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 3b3ba15..2ae84ad 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 >= 0 && j >= start; j--)   vfio_msi_set_vector_signal(vdev, j, -1, msix);   }