Re: [PATCH 7/9] virtio-pci: harden INTX interrupts

[Jason Wang <jasowang@redhat.com>]

On Mon, Sep 13, 2021 at 3:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Sep 13, 2021 at 02:45:38PM +0800, Jason Wang wrote:
> > On Mon, Sep 13, 2021 at 2:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Sep 13, 2021 at 02:36:54PM +0800, Jason Wang wrote:
> > > > On Mon, Sep 13, 2021 at 2:33 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Mon, Sep 13, 2021 at 01:53:51PM +0800, Jason Wang wrote:
> > > > > > This patch tries to make sure the virtio interrupt handler for INTX
> > > > > > won't be called after a reset and before virtio_device_ready(). We
> > > > > > can't use IRQF_NO_AUTOEN since we're using shared interrupt
> > > > > > (IRQF_SHARED). So this patch tracks the INTX enabling status in a new
> > > > > > intx_soft_enabled variable and toggle it during in
> > > > > > vp_disable/enable_vectors(). The INTX interrupt handler will check
> > > > > > intx_soft_enabled before processing the actual interrupt.
> > > > > >
> > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > >
> > > > >
> > > > > Not all that excited about all the memory barriers for something
> > > > > that should be an extremely rare event (for most kernels -
> > > > > literally once per boot). Can't we do better?
> > > >
> > > > I'm not sure, but do we need to care about the slow path (INTX)?
> > >
> > > Otherwise we won't try to support this, right?
> >
> > Sorry, what I meant is "do we need to care about the performance of
> > the slow path".
> >
> > >
> > > > (Or do you have a better approach?)
> > > >
> > > > Thanks
> > >
> > > Don't know really, maybe rcu or whatever?
> >
> > I am sure it's worth it to bother since it's the slow path.
> >
> > > But let's try to be much more specific - is there anything
> > > specific we are trying to protect against here?
> >
> > The unexpected calling of the vring or config interrupt handler. (The
> > same as MSI-X, e.g the untrusted device can send irq at any time).
> >
> > Thanks
>
> And so, does this do more than crash the guest?  Hypervisors
> already can do that ...

Yes, so for DOS and shutdown it should be fine, but for other kinds of
crash, it would be very hard to say what can happen (e.g manipulating
SWIOTLB or page table etc). So it's better to avoid non DOS crashes.

Thanks

>
>
> > >
> > >
> > >
> > > > >
> > > > > > ---
> > > > > >  drivers/virtio/virtio_pci_common.c | 18 ++++++++++++++++--
> > > > > >  drivers/virtio/virtio_pci_common.h |  1 +
> > > > > >  2 files changed, 17 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > index 0b9523e6dd39..835197151dc1 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > @@ -30,8 +30,12 @@ void vp_disable_vectors(struct virtio_device *vdev)
> > > > > >       struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > >       int i;
> > > > > >
> > > > > > -     if (vp_dev->intx_enabled)
> > > > > > +     if (vp_dev->intx_enabled) {
> > > > > > +             vp_dev->intx_soft_enabled = false;
> > > > > > +             /* ensure the vp_interrupt see this intx_soft_enabled value */
> > > > > > +             smp_wmb();
> > > > > >               synchronize_irq(vp_dev->pci_dev->irq);
> > > > > > +     }
> > > > > >
> > > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > >               disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > @@ -43,8 +47,12 @@ void vp_enable_vectors(struct virtio_device *vdev)
> > > > > >       struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > >       int i;
> > > > > >
> > > > > > -     if (vp_dev->intx_enabled)
> > > > > > +     if (vp_dev->intx_enabled) {
> > > > > > +             vp_dev->intx_soft_enabled = true;
> > > > > > +             /* ensure the vp_interrupt see this intx_soft_enabled value */
> > > > > > +             smp_wmb();
> > > > > >               return;
> > > > > > +     }
> > > > > >
> > > > > >       for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > >               enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > @@ -97,6 +105,12 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
> > > > > >       struct virtio_pci_device *vp_dev = opaque;
> > > > > >       u8 isr;
> > > > > >
> > > > > > +     if (!vp_dev->intx_soft_enabled)
> > > > > > +             return IRQ_NONE;
> > > > > > +
> > > > > > +     /* read intx_soft_enabled before read others */
> > > > > > +     smp_rmb();
> > > > > > +
> > > > > >       /* reading the ISR has the effect of also clearing it so it's very
> > > > > >        * important to save off the value. */
> > > > > >       isr = ioread8(vp_dev->isr);
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> > > > > > index a235ce9ff6a5..3c06e0f92ee4 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.h
> > > > > > +++ b/drivers/virtio/virtio_pci_common.h
> > > > > > @@ -64,6 +64,7 @@ struct virtio_pci_device {
> > > > > >       /* MSI-X support */
> > > > > >       int msix_enabled;
> > > > > >       int intx_enabled;
> > > > > > +     bool intx_soft_enabled;
> > > > > >       cpumask_var_t *msix_affinity_masks;
> > > > > >       /* Name strings for interrupts. This size should be enough,
> > > > > >        * and I'm too lazy to allocate each name separately. */
> > > > > > --
> > > > > > 2.25.1
> > > > >
> > >
>