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, 
> > >
> > 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?

> > > >      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.

Best regards,
-Gonglei