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

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

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.

> @@ -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.

I'm not seeing that there's an issue on either path here, please
elaborate if you still disagree.  Thanks,

Alex