Would any of the PCI people like to comment on this? It's been reviewed, I can take it via target-arm if you prefer.
thanks -- PMM On Fri, 7 Nov 2025 at 13:10, Peter Maydell <[email protected]> wrote: > > msix_init() and msix_init_exclusive_bar() take an "unsigned short" > argument for the number of MSI-X vectors to try to use. This is big > enough for the maximum permitted number of vectors, which is 2048. > Unfortunately, we have several devices (most notably virtio) which > allow the user to specify the desired number of vectors, and which > use uint32_t properties for this. If the user sets the property to a > value that is too big for a uint16_t, the value will be truncated > when it is passed to msix_init(), and msix_init() may then return > success if the truncated value is a valid one. > > The resulting mismatch between the number of vectors the msix code > thinks the device has and the number of vectors the device itself > thinks it has can cause assertions, such as the one in issue 2631, > where "-device virtio-mouse-pci,vectors=19923041" is interpreted by > msix as "97 vectors" and by the virtio-pci layer as "19923041 > vectors"; a guest attempt to access vector 97 thus passes the > virtio-pci bounds checking and hits an essertion in > msix_vector_use(). > > Avoid this by making msix_init() and its wrapper function > msix_init_exclusive_bar() take the number of vectors as a uint32_t. > The erroneous command line will now produce the warning > > qemu-system-i386: -device virtio-mouse-pci,vectors=19923041: > warning: unable to init msix vectors to 19923041 > > and proceed without crashing. (The virtio device warns and falls > back to not using MSIX, rather than complaining that the option is > not a valid value this is the same as the existing behaviour for > values that are beyond the MSI-X maximum possible value but fit into > a 16-bit integer, like 2049.) > > To ensure this doesn't result in potential overflows in calculation > of the BAR size in msix_init_exclusive_bar(), we duplicate the > nentries error-check from msix_init() at the top of > msix_init_exclusive_bar(), so we know nentries is sane before we > start using it. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2631 > Signed-off-by: Peter Maydell <[email protected]> > --- > Technically this fixes an assertion, but only if the command > line is daft, so I didn't think it worth backporting to stable. > --- > include/hw/pci/msix.h | 4 ++-- > hw/pci/msix.c | 10 ++++++++-- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h > index 11ef9454c13..551a2bcfe73 100644 > --- a/include/hw/pci/msix.h > +++ b/include/hw/pci/msix.h > @@ -7,12 +7,12 @@ > > void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg); > MSIMessage msix_get_message(PCIDevice *dev, unsigned int vector); > -int msix_init(PCIDevice *dev, unsigned short nentries, > +int msix_init(PCIDevice *dev, uint32_t nentries, > MemoryRegion *table_bar, uint8_t table_bar_nr, > unsigned table_offset, MemoryRegion *pba_bar, > uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos, > Error **errp); > -int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > +int msix_init_exclusive_bar(PCIDevice *dev, uint32_t nentries, > uint8_t bar_nr, Error **errp); > > void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int > len); > diff --git a/hw/pci/msix.c b/hw/pci/msix.c > index 8c7f6709e2a..b35476d0577 100644 > --- a/hw/pci/msix.c > +++ b/hw/pci/msix.c > @@ -318,7 +318,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned > nentries) > * also means a programming error, except device assignment, which can check > * if a real HW is broken. > */ > -int msix_init(struct PCIDevice *dev, unsigned short nentries, > +int msix_init(struct PCIDevice *dev, uint32_t nentries, > MemoryRegion *table_bar, uint8_t table_bar_nr, > unsigned table_offset, MemoryRegion *pba_bar, > uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos, > @@ -392,7 +392,7 @@ int msix_init(struct PCIDevice *dev, unsigned short > nentries, > return 0; > } > > -int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > +int msix_init_exclusive_bar(PCIDevice *dev, uint32_t nentries, > uint8_t bar_nr, Error **errp) > { > int ret; > @@ -401,6 +401,12 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned > short nentries, > uint32_t bar_pba_offset = bar_size / 2; > uint32_t bar_pba_size = QEMU_ALIGN_UP(nentries, 64) / 8; > > + /* Sanity-check nentries before we use it in BAR size calculations */ > + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) { > + error_setg(errp, "The number of MSI-X vectors is invalid"); > + return -EINVAL; > + } > + > /* > * Migration compatibility dictates that this remains a 4k > * BAR with the vector table in the lower half and PBA in > -- > 2.43.0
