On Tue, 11 Oct 2016 22:04:08 +0200
Vlad Tsyrklevich <vlad@xxxxxxxxxxxxxxx> wrote:
> On Tue, Oct 11, 2016 at 6:10 PM, Alex Williamson
> <alex.williamson@xxxxxxxxxx> wrote:
> > On Tue, 11 Oct 2016 13:02:19 +0200
> > Vlad Tsyrklevich <vlad@xxxxxxxxxxxxxxx> 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 <vlad@xxxxxxxxxxxxxxx>
> >>
> >> ---
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
