Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Oct 17, 2011 at 11:27:40AM +0200, Jan Kiszka wrote:
> Only accesses to the MSI-X table must trigger a call to
> msix_handle_mask_update or a notifier invocation.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Why would msix_mmio_write be called on an access
outside the table?

> ---
>  hw/msix.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 2c4de21..33cb716 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -264,18 +264,22 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
>  {
>      PCIDevice *dev = opaque;
>      unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3;
> -    int vector = offset / PCI_MSIX_ENTRY_SIZE;
> +    unsigned int vector = offset / PCI_MSIX_ENTRY_SIZE;

Why the int/unsigned change? this has no chance to overflow, and using
unsigned causes signed/unsigned comparison below,
and unsigned/signed conversion on calls such as msix_is_masked.

>      int was_masked = msix_is_masked(dev, vector);
>      pci_set_long(dev->msix_table_page + offset, val);
>      if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>          kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, vector));
>      }

I would say if we need to check the address, check it first thing
and return if the address is out of a sensible range.
For example, are you worried about kvm_msix_update calls with
a sensible mask?

> -    if (was_masked != msix_is_masked(dev, vector) && dev->msix_mask_notifier) {
> -        int r = dev->msix_mask_notifier(dev, vector,
> -					msix_is_masked(dev, vector));
> -        assert(r >= 0);
> +
> +    if (vector < dev->msix_entries_nr) {
> +        if (was_masked != msix_is_masked(dev, vector) &&
> +            dev->msix_mask_notifier) {
> +            int r = dev->msix_mask_notifier(dev, vector,
> +                                            msix_is_masked(dev, vector));
> +            assert(r >= 0);
> +        }
> +        msix_handle_mask_update(dev, vector);
>      }
> -    msix_handle_mask_update(dev, vector);
>  }
>  
>  static const MemoryRegionOps msix_mmio_ops = {
> -- 
> 1.7.3.4