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

[Gonglei (Arei)]

> > 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.
> 
No, I haven't tried it use vfio-pci. Maybe I will have a try.

> > 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?
> 
Actually I move the judge in function assigned_dev_register_msix_mmio.
Because assigned_dev_register_msix_mmio do not address the return value,
if dev->msix_table is null, this will result a segfault. Right?

> >      memset(dev->msix_table, 0, MSIX_PAGE_SIZE);
> 
> This memset may no longer cover the entire table
> 
Yeah, this memset is useless. Do it in assigned_dev_register_msix_mmio.

> >
> >      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);
> 
Nice. I don't know the macro before. Thank you so much!
> > +    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.
> 
Nice catch.
> As noted by other comments, the munmap() size isn't addressed.
> 
Get it.
> 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,
> 
I will have a try. Alex, What's your opinion about my former letter about the 
MSI-X maximum entry. 
Thanks,

Best regards,
-Gonglei