From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alex Williamson <alex.williamson@redhat.com>
Subject: Re: [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure
Date: Tue, 11 Oct 2016 10:10:31 -0600
Message-ID: <20161011101031.4cdd6647@t450s.home>
References: <1476183739-35210-1-git-send-email-vlad@tsyrklevich.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: kvm@vger.kernel.org
To: Vlad Tsyrklevich <vlad@tsyrklevich.net>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:52872 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752323AbcJKQWc (ORCPT <rfc822;kvm@vger.kernel.org>);
        Tue, 11 Oct 2016 12:22:32 -0400
In-Reply-To: <1476183739-35210-1-git-send-email-vlad@tsyrklevich.net>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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