Re: [Qemu-devel] [RFC PATCH]pci-assign: Fix memory out of bound when MSI-X table not fit in a single page

[Michael S. Tsirkin]

On Tue, Apr 01, 2014 at 03:23:36PM +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.
> 
> 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;
> -    }
> -
>      memset(dev->msix_table, 0, MSIX_PAGE_SIZE);
> 
>      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;
> +    }

It's usually not a good idea to special-case corner-cases like this.


> +
> +    size = MSIX_PAGE_SIZE * nr_pages;

Just use ROUND_UP?

> +    dev->msix_table = mmap(NULL, size, PROT_READ|PROT_WRITE,
>                             MAP_ANONYMOUS|MAP_PRIVATE, 0, 0);

Need to fix unmap as well?

>      if (dev->msix_table == MAP_FAILED) {
>          error_report("fail allocate msix_table! %s", strerror(errno));
>          return -EFAULT;
>      }
> +    if (!dev->msix_table) {
> +        return -EFAULT;
> +    }
> 
> +    memset(dev->msix_table, 0, size);
>      assigned_dev_msix_reset(dev);
> 
>      memory_region_init_io(&dev->mmio, OBJECT(dev), 
> &assigned_dev_msix_mmio_ops,
> -- 
> 1.6.0.2
> 
> Best regards,
> -Gonglei
>