Re: [PATCH v2] vfio/pci: Fix integer overflows, bitmask check

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

On Wed, 12 Oct 2016 17:06:15 +0200
Vlad Tsyrklevich <vlad@tsyrklevich.net> wrote:

> On Wed, Oct 12, 2016 at 4:51 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Wed, 12 Oct 2016 09:53:49 +0200
> > Vlad Tsyrklevich <vlad@tsyrklevich.net> wrote:
> >  
> >> The VFIO_DEVICE_SET_IRQS ioctl did not sufficiently sanitize
> >> user-supplied integers, potentially allowing arbitrary memory writes.
> >> This patch adds appropriate integer checks, and also verifies that only
> >> a single element in the VFIO_IRQ_SET_DATA_TYPE_MASK bitmask is set.
> >> VFIO_IRQ_SET_ACTION_TYPE_MASK is already correctly checked later in
> >> vfio_pci_set_irqs_ioctl().
> >>
> >> Signed-off-by: Vlad Tsyrklevich <vlad@tsyrklevich.net>
> >> ---
> >>  drivers/vfio/pci/vfio_pci.c       | 26 +++++++++++++++++---------
> >>  drivers/vfio/pci/vfio_pci_intrs.c |  2 +-
> >>  2 files changed, 18 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >> index d624a52..44de13c 100644
> >> --- a/drivers/vfio/pci/vfio_pci.c
> >> +++ b/drivers/vfio/pci/vfio_pci.c
> >> @@ -829,6 +829,7 @@ static long vfio_pci_ioctl(void *device_data,
> >>
> >>       } else if (cmd == VFIO_DEVICE_SET_IRQS) {
> >>               struct vfio_irq_set hdr;
> >> +             size_t size;
> >>               u8 *data = NULL;
> >>               int ret = 0;
> >>
> >> @@ -838,20 +839,27 @@ 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;
> >>
> >> -             if (!(hdr.flags & VFIO_IRQ_SET_DATA_NONE)) {
> >> -                     size_t size;
> >> -                     int max = vfio_pci_get_irq_count(vdev, hdr.index);
> >> +             switch (hdr.flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
> >> +             case VFIO_IRQ_SET_DATA_NONE:
> >> +                     size = 0;
> >> +                     break;
> >> +             case VFIO_IRQ_SET_DATA_BOOL:
> >> +                     size = sizeof(uint8_t);
> >> +                     break;
> >> +             case VFIO_IRQ_SET_DATA_EVENTFD:
> >> +                     size = sizeof(int32_t);
> >> +                     break;
> >> +             default:
> >> +                     return -EINVAL;
> >> +             }
> >>
> >> -                     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 (size) {
> >> +                     int max = vfio_pci_get_irq_count(vdev, hdr.index);  
> >
> >
> > Why not pull this out too so we can fully validate start/count for the
> > DATA_NONE case?  
> 
> I'm not familiar with the vfio subsystem so I wasn't sure if the check
> against vfio_pci_get_irq_count() was also valid for the DATA_NONE
> case. It turns out that the ioctls also all check start/count as well
> so it's not strictly necessary, but I'm happy to also break out that
> case here.

It feels like pulling vfio_pci_get_irq_count() up allows us to more
strictly validate the range, not just sanitize it for overflow.  Maybe
that leads to some redundancy later, but we can fix that with future
patches if it seems necessary.
 
> >>
> >>                       if (hdr.argsz - minsz < hdr.count * size ||
> >>                           hdr.start >= max || hdr.start +
> >> hdr.count > max) diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
> >> b/drivers/vfio/pci/vfio_pci_intrs.c index c2e6089..1c46045 100644
> >> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> >> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> >> @@ -256,7 +256,7 @@ static int vfio_msi_enable(struct
> >> vfio_pci_device *vdev, int nvec, bool msix) if (!is_irq_none(vdev))
> >>               return -EINVAL;
> >>
> >> -     vdev->ctx = kzalloc(nvec * sizeof(struct vfio_pci_irq_ctx),
> >> GFP_KERNEL);
> >> +     vdev->ctx = kcalloc(nvec, sizeof(struct vfio_pci_irq_ctx),
> >> GFP_KERNEL); if (!vdev->ctx)
> >>               return -ENOMEM;  
> >
> > Unrelated change?  
> 
> I put it in this patch because kcalloc checks against integer overflow
> and part of the cause of the issue I identified last time was caused
> by the fact that nvec * sizeof(struct vfio_pci_irq_ctx) could be made
> to overflow. That integer overflow is no longer possible with the
> other changes I've made in this patch, but I figured it would be a
> good protection from other integer issues as well. Should have called
> that out more clearly.

Ah, I see.  Yes, please note this in the commit log.  Thanks,

Alex