qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH]pci-assign: Fix memory out of bound when MSI


From: Alex Williamson
Subject: Re: [Qemu-devel] [RFC PATCH]pci-assign: Fix memory out of bound when MSI-X table not fit in a single page
Date: Tue, 01 Apr 2014 21:45:18 -0600

On Tue, 2014-04-01 at 15:23 +0000, Gonglei (Arei) wrote:
> Hi,
> 
> I have a problem about SR-IOV pass-through. 
> 
> The PF is Emulex Corporation OneConnect NIC (Lancer)(rev 10),
> and the VF pci config is as follow:
> 
> LINUX:/sys/bus/pci/devices/0000:04:00.6 # hexdump config
> 0000000 ffff ffff 0000 0010 0010 0200 0000 0080
> 0000010 0000 0000 0000 0000 0000 0000 0000 0000
> 0000020 0000 0000 0000 0000 0000 0000 10df e264
> 0000030 0000 0000 0054 0000 0000 0000 0000 0000
> 0000040 0000 0000 0008 0000 0000 0000 0000 0000
> 0000050 0000 0000 6009 0008 2b41 c002 0000 0000
> 0000060 7805 018a 0000 0000 0000 0000 0000 0000
> 0000070 0000 0000 0000 0000 8411 03ff 4000 0000
> 0000080 3400 0000 9403 0000 0000 0000 0000 0000
> 0000090 0000 0000 0010 0002 8724 1000 0000 0000
> 00000a0 dc83 0041 0000 0000 0000 0000 0000 0000
> 00000b0 0000 0000 0000 0000 001f 0010 0000 0000
> 00000c0 000e 0000 0000 0000 0000 0000 0000 0000
> 00000d0 0000 0000 0000 0000 0000 0000 0000 0000
> 
> We can see the msix_max is 0x3ff and msix_table_entry is 0x4000 (4 pages). 
> But QEMU 
> only mmap MSIX_PAGE_SIZE memory for all pci devices in funciton 
> assigned_dev_register_msix_mmio, 
> meanwhile the set the one page memmory to zero, so the rest memory will be 
> random value
> (maybe etnry.data is not 0). 
> 
> In function assigned_dev_update_msix_mmio maybe occur the issue of entry_nr > 
> 256, 
> and the kmod reports the EINVAL error.
> 
> My patch fix this issue which alloc memory according to the real size of pci 
> device config.
> 
> Any ideas? Thnaks.

Isn't this already fixed if you use vfio-pci?  pci-assign is not well
supported anymore.

> Signed-off-by: Gonglei <address@hidden>
> ---
>  hw/i386/kvm/pci-assign.c |   24 +++++++++++++++++++-----
>  1 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index a825871..daa191c 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -1591,10 +1591,6 @@ static void assigned_dev_msix_reset(AssignedDevice 
> *dev)
>      MSIXTableEntry *entry;
>      int i;
> 
> -    if (!dev->msix_table) {
> -        return;
> -    }
> -

How would this not result in a segfault if the assigned device doesn't
support msix?

>      memset(dev->msix_table, 0, MSIX_PAGE_SIZE);

This memset may no longer cover the entire table

> 
>      for (i = 0, entry = dev->msix_table; i < dev->msix_max; i++, entry++) {
> @@ -1604,13 +1600,31 @@ static void assigned_dev_msix_reset(AssignedDevice 
> *dev)
> 
>  static int assigned_dev_register_msix_mmio(AssignedDevice *dev)
>  {
> -    dev->msix_table = mmap(NULL, MSIX_PAGE_SIZE, PROT_READ|PROT_WRITE,
> +    int nr_pages;
> +    int size;
> +    int entry_per_page = MSIX_PAGE_SIZE / sizeof(struct MSIXTableEntry);
> +    
> +    if (dev->msix_max > entry_per_page) {
> +        nr_pages = dev->msix_max / entry_per_page;
> +        if (dev->msix_max % entry_per_page) {
> +            nr_pages += 1;
> +        }
> +    } else {
> +        nr_pages = 1;
> +    }
> +
> +    size = MSIX_PAGE_SIZE * nr_pages;

Agree with the ROUND_UP comments:

size = ROUND_UP(dev->msix_max * sizeof(struct MSIXTableEntry), MSIX_PAGE_SIZE);

> +    dev->msix_table = mmap(NULL, size, PROT_READ|PROT_WRITE,
>                             MAP_ANONYMOUS|MAP_PRIVATE, 0, 0);
>      if (dev->msix_table == MAP_FAILED) {
>          error_report("fail allocate msix_table! %s", strerror(errno));
>          return -EFAULT;
>      }
> +    if (!dev->msix_table) {
> +        return -EFAULT;
> +    }

This is a bogus test for mmap(2)

> 
> +    memset(dev->msix_table, 0, size);

This is unnecessary when assigned_dev_msix_reset() is fixed to memset
the whole mmap.

>      assigned_dev_msix_reset(dev);
> 
>      memory_region_init_io(&dev->mmio, OBJECT(dev), 
> &assigned_dev_msix_mmio_ops,

This memory_region_init_io also requires a size parameter that isn't
updated for the new size.

As noted by other comments, the munmap() size isn't addressed.

IMHO, the correct way to fix this would be to update pci-assign to use
the msix infrastructure, but legacy KVM assignment is being phased out
and replaced by VFIO.  Is there something that ties you to pci-assign
instead of using vfio-pci?  Thanks,

Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]