Re: [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 11 Oct 2016 22:04:08 +0200
Vlad Tsyrklevich <vlad@tsyrklevich.net> wrote:

> On Tue, Oct 11, 2016 at 6:10 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Tue, 11 Oct 2016 13:02:19 +0200
> > Vlad Tsyrklevich <vlad@tsyrklevich.net> 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@tsyrklevich.net>
> >>
> >> ---
> >>  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