From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Williamson Subject: Re: [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure Date: Tue, 11 Oct 2016 16:39:57 -0600 Message-ID: <20161011163957.2cadbc69@t450s.home> References: <1476183739-35210-1-git-send-email-vlad@tsyrklevich.net> <20161011101031.4cdd6647@t450s.home> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Vlad Tsyrklevich Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55372 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754537AbcJKWj6 (ORCPT ); Tue, 11 Oct 2016 18:39:58 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Tue, 11 Oct 2016 22:04:08 +0200 Vlad Tsyrklevich wrote: > On Tue, Oct 11, 2016 at 6:10 PM, Alex Williamson > wrote: > > On Tue, 11 Oct 2016 13:02:19 +0200 > > Vlad Tsyrklevich wrote: > > > >> The VFIO_DEVICE_SET_IRQS and VFIO_DEVICE_GET_PCI_HOT_RESET_INFO ioctls > >> do not sufficiently sanitize user-supplied integers, allowing users to > >> read arbitrary amounts of kernel heap memory or cause a crash. > >> > >> Signed-off-by: Vlad Tsyrklevich > >> > >> --- > >> drivers/vfio/pci/vfio_pci.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > >> index d624a52..c3fbfb8 100644 > >> --- a/drivers/vfio/pci/vfio_pci.c > >> +++ b/drivers/vfio/pci/vfio_pci.c > >> @@ -838,6 +838,7 @@ static long vfio_pci_ioctl(void *device_data, > >> return -EFAULT; > >> > >> if (hdr.argsz < minsz || hdr.index >= VFIO_PCI_NUM_IRQS || > >> + hdr.count >= (U32_MAX - hdr.start) || > >> hdr.flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK | > >> VFIO_IRQ_SET_ACTION_TYPE_MASK)) > >> return -EINVAL; > > > > > > Isn't the issue you're trying to solve here caught in the code block > > below this? ie. > > > > if (!(hdr.flags & VFIO_IRQ_SET_DATA_NONE)) { > > size_t size; > > int max = vfio_pci_get_irq_count(vdev, hdr.index); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > if (hdr.flags & VFIO_IRQ_SET_DATA_BOOL) > > size = sizeof(uint8_t); > > else if (hdr.flags & VFIO_IRQ_SET_DATA_EVENTFD) > > size = sizeof(int32_t); > > else > > return -EINVAL; > > > > if (hdr.argsz - minsz < hdr.count * size || > > hdr.start >= max || hdr.start + hdr.count > max) > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > return -EINVAL; > > > > data = memdup_user((void __user *)(arg + minsz), > > hdr.count * size); > > if (IS_ERR(data)) > > return PTR_ERR(data); > > } > > > > In fact we're doing better than allowing U32_MAX, we're testing the > > number of indexes for a given IRQ and making sure the user hasn't > > exceeded that. > > That's true, though strictly speaking integer overflows are still > possible. For example, if hdr.count is set to U32_MAX, hdr.start + > hdr.count will overflow and wrap around to hdr.start - 1. That won't > really matter if VFIO_IRQ_SET_DATA_NONE is cleared since memdup_user > should fail early; however, if DATA_NONE is set you could reach the > following code in vfio_pci_set_msi_trigger(): > > if (!irq_is(vdev, index) || start + count > vdev->num_ctx) > return -EINVAL; > > for (i = start; i < start + count; i++) { > if (!vdev->ctx[i].trigger) > continue; > if (flags & VFIO_IRQ_SET_DATA_NONE) { > eventfd_signal(vdev->ctx[i].trigger, 1); > } else if (flags & VFIO_IRQ_SET_DATA_BOOL) { > uint8_t *bools = data; > if (bools[i - start]) > eventfd_signal(vdev->ctx[i].trigger, 1); > } > } > > In this case you could hand pick values of start and count such that > start + count overflow to be less than vdev->num_ctx but still allow > you to access/write to arbitrary offsets from vdev->ctx. Disallowing > integer overflows for any code path reachable through > vfio_pci_set_irqs_ioctl() is a stronger solution than allowing some > overflows by adding the start >= max comparison. Yes, let's sanitize regardless of the data type/presence flags. We can still use vfio_pci_get_irq_count() to be much more precise about it. > While I was writing this out I think I also found another potential > issue. I don't believe anything checks that only one of DATA_NONE, > DATA_BOOL, DATA_EVENTFD is set, so if you set flags to DATA_NONE | > DATA_EVENTFD, you can get around the start and count checks you noted > above. You could reach the following in vfio_pci_set_msi_trigger(): > > if (flags & VFIO_IRQ_SET_DATA_EVENTFD) { > int32_t *fds = data; > int ret; > > if (vdev->irq_type == index) > return vfio_msi_set_block(vdev, start, count, > fds, msix); > > ret = vfio_msi_enable(vdev, start + count, msix); > if (ret) > return ret; > > At this point start and count have not been validated at all. Even if > you add the patch I sent above you could still reach an integer > overflow in vfio_msi_enable(): > > static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix) > { > ... > vdev->ctx = kzalloc(nvec * sizeof(struct vfio_pci_irq_ctx), GFP_KERNEL); > if (!vdev->ctx) > return -ENOMEM; > > /* return the number of supported vectors if we can't get all: */ > ret = pci_alloc_irq_vectors(pdev, 1, nvec, flag); > if (ret < nvec) { > if (ret > 0) > pci_free_irq_vectors(pdev); > kfree(vdev->ctx); > return ret; > } > > On 32-bit systems you could pick values of start and count such that > nvec [start + count] * sizeof(struct vfio_pci_irq_ctx) overflows. The > kzalloc allocation would succeed, but pci_alloc_irq_vectors() would > fail. In the error handler, it would free vdev->ctx but not set the > pointer to NULL. Future calls to the vfio ioctls could lead to a > use-after-free of vdev->ctx. I just found this and haven't had time to > convince myself if it's possible, does that look reasonable? Seems plausible, we should make sure that both VFIO_IRQ_SET_DATA_TYPE_MASK and VFIO_IRQ_SET_ACTION_TYPE_MASK each only have a single bit set. > >> @@ -909,6 +910,9 @@ static long vfio_pci_ioctl(void *device_data, > >> > >> WARN_ON(!fill.max); /* Should always be at least one > >> */ > >> > >> + if (hdr.count > fill.max) > >> + hdr.count = fill.max; > >> + > >> /* > >> * If there's enough space, fill it now, otherwise > >> return > >> * -ENOSPC and the number of devices affected. > > > > hdr.count is an output for this ioctl, not an input. hdr.count is > > set on all success paths and on the -ENOSPC path, the user cannot > > rely on a value for any other error cases. > > You're completely right. I had an error in my static analysis and > misunderstood the control-flow on manual inspection. Sorry for the > noise. > > > I'm not seeing that there's an issue on either path here, please > > elaborate if you still disagree. Thanks, > > If the two issues I noted above look legitimate to you, I'm happy to > submit an updated patch. They do, patch away. Thanks! Alex