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

[Alex Williamson]

On Wed, 2014-04-02 at 08:50 +0000, Gonglei (Arei) wrote:
> Hi, 
> > > >
> > > 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?
> > 
> > I see the confusion, there is a bug there but I think it should be fixed
> > by setting msix_table to NULL in the MAP_FAILED case.  The intent of
> > assigned_dev_msix_reset() is to put the msix table in the state it would
> > be in at reset.  Without a memset() that doesn't happen.  I believe the
> > reason we test msix_table in assigned_dev_msix_reset() is so that the
> > function could be called from anywhere, such as reset_assigned_device()
> > even though it's currently not called from there.  So, if the memset()
> > gets removed, then the whole function should be dissolved.  I'd prefer
> > to keep it and store or recalculate the size for memset.
> > 
> Yep, agreed. Thank you so much. I want to post a formal patch, can I?

Of course, bug fixes are always welcome.  Features and significant code
churn to pci-assign probably require some discussion as we'd really like
to see it phased out in favor of vfio-pci.

> > > > >      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.
> > 
> > Not useless, see above.
> > 
> > > > >
> > > > >      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.
> > 
> > From other thread:
> > 
> > > BTW, do you think the KVM should upsize the max support MSI-X entry to
> > 2048.
> > > Because the MSI-X supports a maximum table size of 2048 entries, which is
> > descript in
> > > PCI specification 3.0 version 6.8.3.2: "MSI-X Configuration".
> > >
> > > The history patch about downsize the MSI-X entry size to 256:
> > >
> > http://thread.gmane.org/gmane.comp.emulators.kvm.devel/38852/focus=388
> > 49
> > 
> > No, I think we should deprecate KVM device assignment and use VFIO.
> > Thanks,
> > 
> OK. I will send the result of using VFIO later.

Great, please let me know if you have any problems.  Thanks,

Alex