Re: [PATCH v2] vfio/pci: Verify each MSI vector to avoid invalid MSI vectors

[Marc Zyngier]

On Sat, 26 Nov 2022 06:33:15 +0000,
"chenxiang (M)" <chenxiang66@hisilicon.com> wrote:
> 
> 
> 在 2022/11/23 20:08, Marc Zyngier 写道:
> > On Wed, 23 Nov 2022 01:42:36 +0000,
> > chenxiang <chenxiang66@hisilicon.com> wrote:
> >> From: Xiang Chen <chenxiang66@hisilicon.com>
> >> 
> >> Currently the number of MSI vectors comes from register PCI_MSI_FLAGS
> >> which should be power-of-2 in qemu, in some scenaries it is not the same as
> >> the number that driver requires in guest, for example, a PCI driver wants
> >> to allocate 6 MSI vecotrs in guest, but as the limitation, it will allocate
> >> 8 MSI vectors. So it requires 8 MSI vectors in qemu while the driver in
> >> guest only wants to allocate 6 MSI vectors.
> >> 
> >> When GICv4.1 is enabled, it iterates over all possible MSIs and enable the
> >> forwarding while the guest has only created some of mappings in the virtual
> >> ITS, so some calls fail. The exception print is as following:
> >> vfio-pci 0000:3a:00.1: irq bypass producer (token 000000008f08224d) 
> >> registration
> >> fails:66311
> >> 
> >> To avoid the issue, verify each MSI vector, skip some operations such as
> >> request_irq() and irq_bypass_register_producer() for those invalid MSI 
> >> vectors.
> >> 
> >> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
> >> ---
> >> I reported the issue at the link:
> >> https://lkml.kernel.org/lkml/87cze9lcut.wl-maz@kernel.org/T/
> >> 
> >> Change Log:
> >> v1 -> v2:
> >> Verify each MSI vector in kernel instead of adding systemcall according to
> >> Mar's suggestion
> >> ---
> >>   arch/arm64/kvm/vgic/vgic-irqfd.c  | 13 +++++++++++++
> >>   arch/arm64/kvm/vgic/vgic-its.c    | 36 
> >> ++++++++++++++++++++++++++++++++++++
> >>   arch/arm64/kvm/vgic/vgic.h        |  1 +
> >>   drivers/vfio/pci/vfio_pci_intrs.c | 33 +++++++++++++++++++++++++++++++++
> >>   include/linux/kvm_host.h          |  2 ++
> >>   5 files changed, 85 insertions(+)
> >> 
> >> diff --git a/arch/arm64/kvm/vgic/vgic-irqfd.c 
> >> b/arch/arm64/kvm/vgic/vgic-irqfd.c
> >> index 475059b..71f6af57 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-irqfd.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-irqfd.c
> >> @@ -98,6 +98,19 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> >>    return vgic_its_inject_msi(kvm, &msi);
> >>   }
> >>   +int kvm_verify_msi(struct kvm *kvm,
> >> +             struct kvm_kernel_irq_routing_entry *irq_entry)
> >> +{
> >> +  struct kvm_msi msi;
> >> +
> >> +  if (!vgic_has_its(kvm))
> >> +          return -ENODEV;
> >> +
> >> +  kvm_populate_msi(irq_entry, &msi);
> >> +
> >> +  return vgic_its_verify_msi(kvm, &msi);
> >> +}
> >> +
> >>   /**
> >>    * kvm_arch_set_irq_inatomic: fast-path for irqfd injection
> >>    */
> >> diff --git a/arch/arm64/kvm/vgic/vgic-its.c 
> >> b/arch/arm64/kvm/vgic/vgic-its.c
> >> index 94a666d..8312a4a 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-its.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> >> @@ -767,6 +767,42 @@ int vgic_its_inject_cached_translation(struct kvm 
> >> *kvm, struct kvm_msi *msi)
> >>    return 0;
> >>   }
> >>   +int vgic_its_verify_msi(struct kvm *kvm, struct kvm_msi *msi)
> >> +{
> >> +  struct vgic_its *its;
> >> +  struct its_ite *ite;
> >> +  struct kvm_vcpu *vcpu;
> >> +  int ret = 0;
> >> +
> >> +  if (!irqchip_in_kernel(kvm) || (msi->flags & ~KVM_MSI_VALID_DEVID))
> >> +          return -EINVAL;
> >> +
> >> +  if (!vgic_has_its(kvm))
> >> +          return -ENODEV;
> >> +
> >> +  its = vgic_msi_to_its(kvm, msi);
> >> +  if (IS_ERR(its))
> >> +          return PTR_ERR(its);
> >> +
> >> +  mutex_lock(&its->its_lock);
> >> +  if (!its->enabled) {
> >> +          ret = -EBUSY;
> >> +          goto unlock;
> >> +  }
> >> +  ite = find_ite(its, msi->devid, msi->data);
> >> +  if (!ite || !its_is_collection_mapped(ite->collection)) {
> >> +          ret = E_ITS_INT_UNMAPPED_INTERRUPT;
> >> +          goto unlock;
> >> +  }
> >> +
> >> +  vcpu = kvm_get_vcpu(kvm, ite->collection->target_addr);
> >> +  if (!vcpu)
> >> +          ret = E_ITS_INT_UNMAPPED_INTERRUPT;
> > I'm sorry, but what does this mean to the caller? This should never
> > leak outside of the ITS code.
> 
> Actually it is already leak outside of ITS code, and please see the
> exception printk (E_ITS_INT_UNMAPPED_INTERRUPT is 0x10307 which is
> equal to 66311):
> 
> vfio-pci 0000:3a:00.1: irq bypass producer (token 000000008f08224d) 
> registration fails:66311
> 

But that's hardly interpreted, which is the whole point. Only zero is
considered a success value.

> > Honestly, the whole things seems really complicated to avoid something
> > that is only a harmless warning .
> 
> It seems also waste some interrupts. Allocating and requesting some
> interrupts but not used.

What makes you think they are not used? A guest can install a mapping
for those at any point. They won't be directly injected, but they will
be delivered to the guest via the normal SW injection mechanism.

        M.

-- 
Without deviation from the norm, progress is not possible.