Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13 2021 at 16:54, Michael S. Tsirkin wrote: > On Mon, Sep 13, 2021 at 09:38:30PM +0200, Thomas Gleixner wrote: >> and disable it again >> before reset() is invoked. That's a question of general robustness and >> not really a question of trusted hypervisors and encrypted guests. > > We can do this for some MSIX interrupts, sure. Not for shared interrupts > though. But you have to make sure that the handler does not run before and after the defined points. And that's even more important for shared because with shared interrupts the interrupt can be raised at any point in time via the other devices which share the line. Thanks, tglx ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
在 2021/9/14 上午6:31, Thomas Gleixner 写道: On Mon, Sep 13 2021 at 16:54, Michael S. Tsirkin wrote: On Mon, Sep 13, 2021 at 09:38:30PM +0200, Thomas Gleixner wrote: On Mon, Sep 13 2021 at 15:07, Jason Wang wrote: On Mon, Sep 13, 2021 at 2:50 PM Michael S. Tsirkin wrote: But doen't "irq is disabled" basically mean "we told the hypervisor to disable the irq"? What extractly prevents hypervisor from sending the irq even if guest thinks it disabled it? More generally, can't we for example blow away the indir_desc array that we use to keep the ctx pointers? Won't that be enough? I'm not sure how it is related to the indirect descriptor but an example is that all the current driver will assume: 1) the interrupt won't be raised before virtio_device_ready() 2) the interrupt won't be raised after reset() If that assumption exists, then you better keep the interrupt line disabled until virtio_device_ready() has completed started not completed. device is allowed to send config interrupts right after DRIVER_OK status is set by virtio_device_ready. Whatever: * Define the exact point from which on the driver is able to handle the interrupt and put the enable after that point * Define the exact point from which on the driver is unable to handle the interrupt and put the disable before that point Yes, this is exactly what this patch (and INTX patch) want to achieve. The driver should only able to handle the interrupt after virtio_device_ready() but before reset(). Thanks The above is blury. and disable it again before reset() is invoked. That's a question of general robustness and not really a question of trusted hypervisors and encrypted guests. We can do this for some MSIX interrupts, sure. Not for shared interrupts though. See my reply to the next patch. The problem is the same: * Define the exact point from which on the driver is able to handle the interrupt and allow the handler to proceed after that point * Define the exact point from which on the driver is unable to handle the interrupt and ensure that the handler denies to proceed before that point Same story just a different mechanism. Thanks, tglx ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13 2021 at 16:54, Michael S. Tsirkin wrote: > On Mon, Sep 13, 2021 at 09:38:30PM +0200, Thomas Gleixner wrote: >> On Mon, Sep 13 2021 at 15:07, Jason Wang wrote: >> > On Mon, Sep 13, 2021 at 2:50 PM Michael S. Tsirkin wrote: >> >> > But doen't "irq is disabled" basically mean "we told the hypervisor >> >> > to disable the irq"? What extractly prevents hypervisor from >> >> > sending the irq even if guest thinks it disabled it? >> >> >> >> More generally, can't we for example blow away the >> >> indir_desc array that we use to keep the ctx pointers? >> >> Won't that be enough? >> > >> > I'm not sure how it is related to the indirect descriptor but an >> > example is that all the current driver will assume: >> > >> > 1) the interrupt won't be raised before virtio_device_ready() >> > 2) the interrupt won't be raised after reset() >> >> If that assumption exists, then you better keep the interrupt line >> disabled until virtio_device_ready() has completed > > started not completed. device is allowed to send > config interrupts right after DRIVER_OK status is set by > virtio_device_ready. Whatever: * Define the exact point from which on the driver is able to handle the interrupt and put the enable after that point * Define the exact point from which on the driver is unable to handle the interrupt and put the disable before that point The above is blury. >> and disable it again >> before reset() is invoked. That's a question of general robustness and >> not really a question of trusted hypervisors and encrypted guests. > > We can do this for some MSIX interrupts, sure. Not for shared interrupts > though. See my reply to the next patch. The problem is the same: * Define the exact point from which on the driver is able to handle the interrupt and allow the handler to proceed after that point * Define the exact point from which on the driver is unable to handle the interrupt and ensure that the handler denies to proceed before that point Same story just a different mechanism. Thanks, tglx ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 09:38:30PM +0200, Thomas Gleixner wrote:
> On Mon, Sep 13 2021 at 15:07, Jason Wang wrote:
> > On Mon, Sep 13, 2021 at 2:50 PM Michael S. Tsirkin wrote:
> >> > But doen't "irq is disabled" basically mean "we told the hypervisor
> >> > to disable the irq"? What extractly prevents hypervisor from
> >> > sending the irq even if guest thinks it disabled it?
> >>
> >> More generally, can't we for example blow away the
> >> indir_desc array that we use to keep the ctx pointers?
> >> Won't that be enough?
> >
> > I'm not sure how it is related to the indirect descriptor but an
> > example is that all the current driver will assume:
> >
> > 1) the interrupt won't be raised before virtio_device_ready()
> > 2) the interrupt won't be raised after reset()
>
> If that assumption exists, then you better keep the interrupt line
> disabled until virtio_device_ready() has completed
started not completed. device is allowed to send
config interrupts right after DRIVER_OK status is set by
virtio_device_ready.
> and disable it again
> before reset() is invoked. That's a question of general robustness and
> not really a question of trusted hypervisors and encrypted guests.
We can do this for some MSIX interrupts, sure. Not for shared interrupts though.
> >> > > > > > > +void vp_disable_vectors(struct virtio_device *vdev)
> >> > > > > > > {
> >> > > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >> > > > > > > int i;
> >> > > > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct
> >> > > > > > > virtio_device *vdev)
> >> > > > > > > synchronize_irq(vp_dev->pci_dev->irq);
>
> Don't you want the same change for non-MSI interrupts?
We can't disable them - these are shared.
> Thanks,
>
> tglx
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13 2021 at 15:07, Jason Wang wrote:
> On Mon, Sep 13, 2021 at 2:50 PM Michael S. Tsirkin wrote:
>> > But doen't "irq is disabled" basically mean "we told the hypervisor
>> > to disable the irq"? What extractly prevents hypervisor from
>> > sending the irq even if guest thinks it disabled it?
>>
>> More generally, can't we for example blow away the
>> indir_desc array that we use to keep the ctx pointers?
>> Won't that be enough?
>
> I'm not sure how it is related to the indirect descriptor but an
> example is that all the current driver will assume:
>
> 1) the interrupt won't be raised before virtio_device_ready()
> 2) the interrupt won't be raised after reset()
If that assumption exists, then you better keep the interrupt line
disabled until virtio_device_ready() has completed and disable it again
before reset() is invoked. That's a question of general robustness and
not really a question of trusted hypervisors and encrypted guests.
>> > > > > > > +void vp_disable_vectors(struct virtio_device *vdev)
>> > > > > > > {
>> > > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> > > > > > > int i;
>> > > > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct
>> > > > > > > virtio_device *vdev)
>> > > > > > > synchronize_irq(vp_dev->pci_dev->irq);
Don't you want the same change for non-MSI interrupts?
Thanks,
tglx
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 3:01 PM Michael S. Tsirkin wrote:
>
> On Mon, Sep 13, 2021 at 02:43:08PM +0800, Jason Wang wrote:
> > On Mon, Sep 13, 2021 at 2:37 PM Michael S. Tsirkin wrote:
> > >
> > > On Mon, Sep 13, 2021 at 02:34:01PM +0800, Jason Wang wrote:
> > > > On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin
> > > > wrote:
> > > > >
> > > > > On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> > > > > > On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin
> > > > > > wrote:
> > > > > > >
> > > > > > > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > > > > > > We used to synchronize pending MSI-X irq handlers via
> > > > > > > > synchronize_irq(), this may not work for the untrusted device
> > > > > > > > which
> > > > > > > > may keep sending interrupts after reset which may lead
> > > > > > > > unexpected
> > > > > > > > results. Similarly, we should not enable MSI-X interrupt until
> > > > > > > > the
> > > > > > > > device is ready. So this patch fixes those two issues by:
> > > > > > > >
> > > > > > > > 1) switching to use disable_irq() to prevent the virtio
> > > > > > > > interrupt
> > > > > > > >handlers to be called after the device is reset.
> > > > > > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > > > > > > >
> > > > > > > > This can make sure the virtio interrupt handler won't be called
> > > > > > > > before
> > > > > > > > virtio_device_ready() and after reset.
> > > > > > > >
> > > > > > > > Signed-off-by: Jason Wang
> > > > > > >
> > > > > > > I don't get the threat model here. Isn't disabling irqs done by
> > > > > > > the
> > > > > > > hypervisor anyway? Is there a reason to trust disable_irq but not
> > > > > > > device reset?
> > > > > >
> > > > > > My understanding is that e.g in the case of SEV/TDX we don't trust
> > > > > > the
> > > > > > hypervisor. So the hypervisor can keep sending interrupts even if
> > > > > > the
> > > > > > device is reset. The guest can only trust its own software interrupt
> > > > > > management logic to avoid call virtio callback in this case.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > Hmm but I don't see how do these patches do this.
> > > > > They call disable_irq but can't the hypervisor keep
> > > > > sending interrupts after disable_irq, too?
> > > >
> > > > Yes, but since the irq is disabled, the vring or config callback won't
> > > > be called in this case.
> > > >
> > > > Thanks
> > >
> > > But doen't "irq is disabled" basically mean "we told the hypervisor
> > > to disable the irq"? What extractly prevents hypervisor from
> > > sending the irq even if guest thinks it disabled it?
> >
> > It can't prevent the hypersior from sending irq. But it can make sure
> > the irq descriptor is disabled (e.g IRQD_IRQ_DISABLED). Is this
> > sufficient?
> >
> > Thanks
>
> Maybe, maybe not ... there's not a lot in the way of
> memory barriers around code using that bit, that's for sure.
Ok, I think the irq core should be robust enough for such unexpected
irq for many years but maybe I was wrong.
But anyhow, the virtio core should be prepared for this, since the irq
core doesn't know when the irq should be raised.
Thanks
> Did anyone look at it from point of view of what
> can a bad interrupt do?
>
>
> > >
> > > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > Cc a bunch more people ...
> > > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > > drivers/virtio/virtio_pci_common.c | 27
> > > > > > > > +--
> > > > > > > > drivers/virtio/virtio_pci_common.h | 6 --
> > > > > > > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > > > > > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > > > > > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > > > > > > b/drivers/virtio/virtio_pci_common.c
> > > > > > > > index b35bb2d57f62..0b9523e6dd39 100644
> > > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > > > > > >"Force legacy mode for transitional virtio 1
> > > > > > > > devices");
> > > > > > > > #endif
> > > > > > > >
> > > > > > > > -/* wait for pending irq handlers */
> > > > > > > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > > > > > +/* disable irq handlers */
> > > > > > > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > > > > > > {
> > > > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > > > int i;
> > > > > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct
> > > > > > > > virtio_device *vdev)
> > > > > > > > synchronize_irq(vp_dev->pci_dev->irq);
> > > > > > > >
> > > > > > > > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > > > -
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 2:50 PM Michael S. Tsirkin wrote:
>
> On Mon, Sep 13, 2021 at 02:37:42AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Sep 13, 2021 at 02:34:01PM +0800, Jason Wang wrote:
> > > On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin
> > > wrote:
> > > >
> > > > On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> > > > > On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin
> > > > > wrote:
> > > > > >
> > > > > > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > > > > > We used to synchronize pending MSI-X irq handlers via
> > > > > > > synchronize_irq(), this may not work for the untrusted device
> > > > > > > which
> > > > > > > may keep sending interrupts after reset which may lead unexpected
> > > > > > > results. Similarly, we should not enable MSI-X interrupt until the
> > > > > > > device is ready. So this patch fixes those two issues by:
> > > > > > >
> > > > > > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > > > > > >handlers to be called after the device is reset.
> > > > > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > > > > > >
> > > > > > > This can make sure the virtio interrupt handler won't be called
> > > > > > > before
> > > > > > > virtio_device_ready() and after reset.
> > > > > > >
> > > > > > > Signed-off-by: Jason Wang
> > > > > >
> > > > > > I don't get the threat model here. Isn't disabling irqs done by the
> > > > > > hypervisor anyway? Is there a reason to trust disable_irq but not
> > > > > > device reset?
> > > > >
> > > > > My understanding is that e.g in the case of SEV/TDX we don't trust the
> > > > > hypervisor. So the hypervisor can keep sending interrupts even if the
> > > > > device is reset. The guest can only trust its own software interrupt
> > > > > management logic to avoid call virtio callback in this case.
> > > > >
> > > > > Thanks
> > > >
> > > > Hmm but I don't see how do these patches do this.
> > > > They call disable_irq but can't the hypervisor keep
> > > > sending interrupts after disable_irq, too?
> > >
> > > Yes, but since the irq is disabled, the vring or config callback won't
> > > be called in this case.
> > >
> > > Thanks
> >
> > But doen't "irq is disabled" basically mean "we told the hypervisor
> > to disable the irq"? What extractly prevents hypervisor from
> > sending the irq even if guest thinks it disabled it?
>
> More generally, can't we for example blow away the
> indir_desc array that we use to keep the ctx pointers?
> Won't that be enough?
I'm not sure how it is related to the indirect descriptor but an
example is that all the current driver will assume:
1) the interrupt won't be raised before virtio_device_ready()
2) the interrupt won't be raised after reset()
All of these above two are not true if we don't trust the device. If
we want to audit all the drivers it would be not trivial and hard to
be correct. So it looks to me it's better to avoid those in the virtio
core.
>
>
> And looking at all this closely, I suspect it is wise to just completely
> disable hotplug/unplug for encrypted guests. Or maybe it's already
> the case?
I think not, it's more about:
1) what happens if the vq interrupt handler is called between
init_vqs() and virito_device_ready()
2) what happens if vq interrupt handler is called after reset
Instead of trying to answer those hard questions, it's better simply
not to have those conditions.
Thanks
>
>
> > > >
> > > >
> > > >
> > > > > >
> > > > > > Cc a bunch more people ...
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > > drivers/virtio/virtio_pci_common.c | 27
> > > > > > > +--
> > > > > > > drivers/virtio/virtio_pci_common.h | 6 --
> > > > > > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > > > > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > > > > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > > > > > b/drivers/virtio/virtio_pci_common.c
> > > > > > > index b35bb2d57f62..0b9523e6dd39 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > > > > >"Force legacy mode for transitional virtio 1
> > > > > > > devices");
> > > > > > > #endif
> > > > > > >
> > > > > > > -/* wait for pending irq handlers */
> > > > > > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > > > > +/* disable irq handlers */
> > > > > > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > > > > > {
> > > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > > int i;
> > > > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct
> > > > > > > virtio_device *vdev)
> > > > > > > synchronize_irq(vp_dev->pci_dev->irq);
> > > > > > >
> > > > > > > for (i = 0; i <
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 02:43:08PM +0800, Jason Wang wrote:
> On Mon, Sep 13, 2021 at 2:37 PM Michael S. Tsirkin wrote:
> >
> > On Mon, Sep 13, 2021 at 02:34:01PM +0800, Jason Wang wrote:
> > > On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin
> > > wrote:
> > > >
> > > > On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> > > > > On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin
> > > > > wrote:
> > > > > >
> > > > > > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > > > > > We used to synchronize pending MSI-X irq handlers via
> > > > > > > synchronize_irq(), this may not work for the untrusted device
> > > > > > > which
> > > > > > > may keep sending interrupts after reset which may lead unexpected
> > > > > > > results. Similarly, we should not enable MSI-X interrupt until the
> > > > > > > device is ready. So this patch fixes those two issues by:
> > > > > > >
> > > > > > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > > > > > >handlers to be called after the device is reset.
> > > > > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > > > > > >
> > > > > > > This can make sure the virtio interrupt handler won't be called
> > > > > > > before
> > > > > > > virtio_device_ready() and after reset.
> > > > > > >
> > > > > > > Signed-off-by: Jason Wang
> > > > > >
> > > > > > I don't get the threat model here. Isn't disabling irqs done by the
> > > > > > hypervisor anyway? Is there a reason to trust disable_irq but not
> > > > > > device reset?
> > > > >
> > > > > My understanding is that e.g in the case of SEV/TDX we don't trust the
> > > > > hypervisor. So the hypervisor can keep sending interrupts even if the
> > > > > device is reset. The guest can only trust its own software interrupt
> > > > > management logic to avoid call virtio callback in this case.
> > > > >
> > > > > Thanks
> > > >
> > > > Hmm but I don't see how do these patches do this.
> > > > They call disable_irq but can't the hypervisor keep
> > > > sending interrupts after disable_irq, too?
> > >
> > > Yes, but since the irq is disabled, the vring or config callback won't
> > > be called in this case.
> > >
> > > Thanks
> >
> > But doen't "irq is disabled" basically mean "we told the hypervisor
> > to disable the irq"? What extractly prevents hypervisor from
> > sending the irq even if guest thinks it disabled it?
>
> It can't prevent the hypersior from sending irq. But it can make sure
> the irq descriptor is disabled (e.g IRQD_IRQ_DISABLED). Is this
> sufficient?
>
> Thanks
Maybe, maybe not ... there's not a lot in the way of
memory barriers around code using that bit, that's for sure.
Did anyone look at it from point of view of what
can a bad interrupt do?
> >
> > > >
> > > >
> > > >
> > > > > >
> > > > > > Cc a bunch more people ...
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > > drivers/virtio/virtio_pci_common.c | 27
> > > > > > > +--
> > > > > > > drivers/virtio/virtio_pci_common.h | 6 --
> > > > > > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > > > > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > > > > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > > > > > b/drivers/virtio/virtio_pci_common.c
> > > > > > > index b35bb2d57f62..0b9523e6dd39 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > > > > >"Force legacy mode for transitional virtio 1
> > > > > > > devices");
> > > > > > > #endif
> > > > > > >
> > > > > > > -/* wait for pending irq handlers */
> > > > > > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > > > > +/* disable irq handlers */
> > > > > > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > > > > > {
> > > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > > int i;
> > > > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct
> > > > > > > virtio_device *vdev)
> > > > > > > synchronize_irq(vp_dev->pci_dev->irq);
> > > > > > >
> > > > > > > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* enable irq handlers */
> > > > > > > +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)
> > > > > > > + return;
> > > > > > > +
> > > > > > > + for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 02:37:42AM -0400, Michael S. Tsirkin wrote:
> On Mon, Sep 13, 2021 at 02:34:01PM +0800, Jason Wang wrote:
> > On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin wrote:
> > >
> > > On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> > > > On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin
> > > > wrote:
> > > > >
> > > > > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > > > > We used to synchronize pending MSI-X irq handlers via
> > > > > > synchronize_irq(), this may not work for the untrusted device which
> > > > > > may keep sending interrupts after reset which may lead unexpected
> > > > > > results. Similarly, we should not enable MSI-X interrupt until the
> > > > > > device is ready. So this patch fixes those two issues by:
> > > > > >
> > > > > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > > > > >handlers to be called after the device is reset.
> > > > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > > > > >
> > > > > > This can make sure the virtio interrupt handler won't be called
> > > > > > before
> > > > > > virtio_device_ready() and after reset.
> > > > > >
> > > > > > Signed-off-by: Jason Wang
> > > > >
> > > > > I don't get the threat model here. Isn't disabling irqs done by the
> > > > > hypervisor anyway? Is there a reason to trust disable_irq but not
> > > > > device reset?
> > > >
> > > > My understanding is that e.g in the case of SEV/TDX we don't trust the
> > > > hypervisor. So the hypervisor can keep sending interrupts even if the
> > > > device is reset. The guest can only trust its own software interrupt
> > > > management logic to avoid call virtio callback in this case.
> > > >
> > > > Thanks
> > >
> > > Hmm but I don't see how do these patches do this.
> > > They call disable_irq but can't the hypervisor keep
> > > sending interrupts after disable_irq, too?
> >
> > Yes, but since the irq is disabled, the vring or config callback won't
> > be called in this case.
> >
> > Thanks
>
> But doen't "irq is disabled" basically mean "we told the hypervisor
> to disable the irq"? What extractly prevents hypervisor from
> sending the irq even if guest thinks it disabled it?
More generally, can't we for example blow away the
indir_desc array that we use to keep the ctx pointers?
Won't that be enough?
And looking at all this closely, I suspect it is wise to just completely
disable hotplug/unplug for encrypted guests. Or maybe it's already
the case?
> > >
> > >
> > >
> > > > >
> > > > > Cc a bunch more people ...
> > > > >
> > > > >
> > > > > > ---
> > > > > > drivers/virtio/virtio_pci_common.c | 27 +--
> > > > > > drivers/virtio/virtio_pci_common.h | 6 --
> > > > > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > > > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > > > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > > > > b/drivers/virtio/virtio_pci_common.c
> > > > > > index b35bb2d57f62..0b9523e6dd39 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > > > >"Force legacy mode for transitional virtio 1
> > > > > > devices");
> > > > > > #endif
> > > > > >
> > > > > > -/* wait for pending irq handlers */
> > > > > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > > > +/* disable irq handlers */
> > > > > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > > > > {
> > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > int i;
> > > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device
> > > > > > *vdev)
> > > > > > synchronize_irq(vp_dev->pci_dev->irq);
> > > > > >
> > > > > > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > +}
> > > > > > +
> > > > > > +/* enable irq handlers */
> > > > > > +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)
> > > > > > + return;
> > > > > > +
> > > > > > + for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > }
> > > > > >
> > > > > > /* the notify function used when creating a virt queue */
> > > > > > @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct
> > > > > > virtio_device *vdev, int nvectors,
> > > > > > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > > > > >"%s-config", name);
> > > > > >
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 2:37 PM Michael S. Tsirkin wrote:
>
> On Mon, Sep 13, 2021 at 02:34:01PM +0800, Jason Wang wrote:
> > On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin wrote:
> > >
> > > On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> > > > On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin
> > > > wrote:
> > > > >
> > > > > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > > > > We used to synchronize pending MSI-X irq handlers via
> > > > > > synchronize_irq(), this may not work for the untrusted device which
> > > > > > may keep sending interrupts after reset which may lead unexpected
> > > > > > results. Similarly, we should not enable MSI-X interrupt until the
> > > > > > device is ready. So this patch fixes those two issues by:
> > > > > >
> > > > > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > > > > >handlers to be called after the device is reset.
> > > > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > > > > >
> > > > > > This can make sure the virtio interrupt handler won't be called
> > > > > > before
> > > > > > virtio_device_ready() and after reset.
> > > > > >
> > > > > > Signed-off-by: Jason Wang
> > > > >
> > > > > I don't get the threat model here. Isn't disabling irqs done by the
> > > > > hypervisor anyway? Is there a reason to trust disable_irq but not
> > > > > device reset?
> > > >
> > > > My understanding is that e.g in the case of SEV/TDX we don't trust the
> > > > hypervisor. So the hypervisor can keep sending interrupts even if the
> > > > device is reset. The guest can only trust its own software interrupt
> > > > management logic to avoid call virtio callback in this case.
> > > >
> > > > Thanks
> > >
> > > Hmm but I don't see how do these patches do this.
> > > They call disable_irq but can't the hypervisor keep
> > > sending interrupts after disable_irq, too?
> >
> > Yes, but since the irq is disabled, the vring or config callback won't
> > be called in this case.
> >
> > Thanks
>
> But doen't "irq is disabled" basically mean "we told the hypervisor
> to disable the irq"? What extractly prevents hypervisor from
> sending the irq even if guest thinks it disabled it?
It can't prevent the hypersior from sending irq. But it can make sure
the irq descriptor is disabled (e.g IRQD_IRQ_DISABLED). Is this
sufficient?
Thanks
>
> > >
> > >
> > >
> > > > >
> > > > > Cc a bunch more people ...
> > > > >
> > > > >
> > > > > > ---
> > > > > > drivers/virtio/virtio_pci_common.c | 27 +--
> > > > > > drivers/virtio/virtio_pci_common.h | 6 --
> > > > > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > > > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > > > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > > > > b/drivers/virtio/virtio_pci_common.c
> > > > > > index b35bb2d57f62..0b9523e6dd39 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > > > >"Force legacy mode for transitional virtio 1
> > > > > > devices");
> > > > > > #endif
> > > > > >
> > > > > > -/* wait for pending irq handlers */
> > > > > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > > > +/* disable irq handlers */
> > > > > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > > > > {
> > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > int i;
> > > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device
> > > > > > *vdev)
> > > > > > synchronize_irq(vp_dev->pci_dev->irq);
> > > > > >
> > > > > > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > +}
> > > > > > +
> > > > > > +/* enable irq handlers */
> > > > > > +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)
> > > > > > + return;
> > > > > > +
> > > > > > + for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > > }
> > > > > >
> > > > > > /* the notify function used when creating a virt queue */
> > > > > > @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct
> > > > > > virtio_device *vdev, int nvectors,
> > > > > > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > > > > >"%s-config", name);
> > > > > > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > > > > > - vp_config_changed, 0, vp_dev->msix_names[v],
>
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 02:34:01PM +0800, Jason Wang wrote:
> On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin wrote:
> >
> > On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> > > On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin
> > > wrote:
> > > >
> > > > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > > > We used to synchronize pending MSI-X irq handlers via
> > > > > synchronize_irq(), this may not work for the untrusted device which
> > > > > may keep sending interrupts after reset which may lead unexpected
> > > > > results. Similarly, we should not enable MSI-X interrupt until the
> > > > > device is ready. So this patch fixes those two issues by:
> > > > >
> > > > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > > > >handlers to be called after the device is reset.
> > > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > > > >
> > > > > This can make sure the virtio interrupt handler won't be called before
> > > > > virtio_device_ready() and after reset.
> > > > >
> > > > > Signed-off-by: Jason Wang
> > > >
> > > > I don't get the threat model here. Isn't disabling irqs done by the
> > > > hypervisor anyway? Is there a reason to trust disable_irq but not
> > > > device reset?
> > >
> > > My understanding is that e.g in the case of SEV/TDX we don't trust the
> > > hypervisor. So the hypervisor can keep sending interrupts even if the
> > > device is reset. The guest can only trust its own software interrupt
> > > management logic to avoid call virtio callback in this case.
> > >
> > > Thanks
> >
> > Hmm but I don't see how do these patches do this.
> > They call disable_irq but can't the hypervisor keep
> > sending interrupts after disable_irq, too?
>
> Yes, but since the irq is disabled, the vring or config callback won't
> be called in this case.
>
> Thanks
But doen't "irq is disabled" basically mean "we told the hypervisor
to disable the irq"? What extractly prevents hypervisor from
sending the irq even if guest thinks it disabled it?
> >
> >
> >
> > > >
> > > > Cc a bunch more people ...
> > > >
> > > >
> > > > > ---
> > > > > drivers/virtio/virtio_pci_common.c | 27 +--
> > > > > drivers/virtio/virtio_pci_common.h | 6 --
> > > > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > > > b/drivers/virtio/virtio_pci_common.c
> > > > > index b35bb2d57f62..0b9523e6dd39 100644
> > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > > >"Force legacy mode for transitional virtio 1 devices");
> > > > > #endif
> > > > >
> > > > > -/* wait for pending irq handlers */
> > > > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > > +/* disable irq handlers */
> > > > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > > > {
> > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > int i;
> > > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device
> > > > > *vdev)
> > > > > synchronize_irq(vp_dev->pci_dev->irq);
> > > > >
> > > > > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > +}
> > > > > +
> > > > > +/* enable irq handlers */
> > > > > +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)
> > > > > + return;
> > > > > +
> > > > > + for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > > }
> > > > >
> > > > > /* the notify function used when creating a virt queue */
> > > > > @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct
> > > > > virtio_device *vdev, int nvectors,
> > > > > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > > > >"%s-config", name);
> > > > > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > > > > - vp_config_changed, 0, vp_dev->msix_names[v],
> > > > > + vp_config_changed, IRQF_NO_AUTOEN,
> > > > > + vp_dev->msix_names[v],
> > > > > vp_dev);
> > > > > if (err)
> > > > > goto error;
> > > > > @@ -160,7 +174,8 @@ static int vp_request_msix_vectors(struct
> > > > > virtio_device *vdev, int nvectors,
> > > > > snprintf(vp_dev->msix_names[v], sizeof
> > > > > *vp_dev->msix_names,
> > > > >
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 2:28 PM Michael S. Tsirkin wrote:
>
> On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> > On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin wrote:
> > >
> > > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > > We used to synchronize pending MSI-X irq handlers via
> > > > synchronize_irq(), this may not work for the untrusted device which
> > > > may keep sending interrupts after reset which may lead unexpected
> > > > results. Similarly, we should not enable MSI-X interrupt until the
> > > > device is ready. So this patch fixes those two issues by:
> > > >
> > > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > > >handlers to be called after the device is reset.
> > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > > >
> > > > This can make sure the virtio interrupt handler won't be called before
> > > > virtio_device_ready() and after reset.
> > > >
> > > > Signed-off-by: Jason Wang
> > >
> > > I don't get the threat model here. Isn't disabling irqs done by the
> > > hypervisor anyway? Is there a reason to trust disable_irq but not
> > > device reset?
> >
> > My understanding is that e.g in the case of SEV/TDX we don't trust the
> > hypervisor. So the hypervisor can keep sending interrupts even if the
> > device is reset. The guest can only trust its own software interrupt
> > management logic to avoid call virtio callback in this case.
> >
> > Thanks
>
> Hmm but I don't see how do these patches do this.
> They call disable_irq but can't the hypervisor keep
> sending interrupts after disable_irq, too?
Yes, but since the irq is disabled, the vring or config callback won't
be called in this case.
Thanks
>
>
>
> > >
> > > Cc a bunch more people ...
> > >
> > >
> > > > ---
> > > > drivers/virtio/virtio_pci_common.c | 27 +--
> > > > drivers/virtio/virtio_pci_common.h | 6 --
> > > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > > b/drivers/virtio/virtio_pci_common.c
> > > > index b35bb2d57f62..0b9523e6dd39 100644
> > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > > >"Force legacy mode for transitional virtio 1 devices");
> > > > #endif
> > > >
> > > > -/* wait for pending irq handlers */
> > > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > > +/* disable irq handlers */
> > > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > > {
> > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > int i;
> > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device
> > > > *vdev)
> > > > synchronize_irq(vp_dev->pci_dev->irq);
> > > >
> > > > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > +}
> > > > +
> > > > +/* enable irq handlers */
> > > > +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)
> > > > + return;
> > > > +
> > > > + for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > > }
> > > >
> > > > /* the notify function used when creating a virt queue */
> > > > @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct
> > > > virtio_device *vdev, int nvectors,
> > > > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > > >"%s-config", name);
> > > > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > > > - vp_config_changed, 0, vp_dev->msix_names[v],
> > > > + vp_config_changed, IRQF_NO_AUTOEN,
> > > > + vp_dev->msix_names[v],
> > > > vp_dev);
> > > > if (err)
> > > > goto error;
> > > > @@ -160,7 +174,8 @@ static int vp_request_msix_vectors(struct
> > > > virtio_device *vdev, int nvectors,
> > > > snprintf(vp_dev->msix_names[v], sizeof
> > > > *vp_dev->msix_names,
> > > >"%s-virtqueues", name);
> > > > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > > > - vp_vring_interrupt, 0,
> > > > vp_dev->msix_names[v],
> > > > + vp_vring_interrupt, IRQF_NO_AUTOEN,
> > > > + vp_dev->msix_names[v],
> > > > vp_dev);
> > > > if (err)
> > > >
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 02:08:02PM +0800, Jason Wang wrote:
> On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin wrote:
> >
> > On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > > We used to synchronize pending MSI-X irq handlers via
> > > synchronize_irq(), this may not work for the untrusted device which
> > > may keep sending interrupts after reset which may lead unexpected
> > > results. Similarly, we should not enable MSI-X interrupt until the
> > > device is ready. So this patch fixes those two issues by:
> > >
> > > 1) switching to use disable_irq() to prevent the virtio interrupt
> > >handlers to be called after the device is reset.
> > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> > >
> > > This can make sure the virtio interrupt handler won't be called before
> > > virtio_device_ready() and after reset.
> > >
> > > Signed-off-by: Jason Wang
> >
> > I don't get the threat model here. Isn't disabling irqs done by the
> > hypervisor anyway? Is there a reason to trust disable_irq but not
> > device reset?
>
> My understanding is that e.g in the case of SEV/TDX we don't trust the
> hypervisor. So the hypervisor can keep sending interrupts even if the
> device is reset. The guest can only trust its own software interrupt
> management logic to avoid call virtio callback in this case.
>
> Thanks
Hmm but I don't see how do these patches do this.
They call disable_irq but can't the hypervisor keep
sending interrupts after disable_irq, too?
> >
> > Cc a bunch more people ...
> >
> >
> > > ---
> > > drivers/virtio/virtio_pci_common.c | 27 +--
> > > drivers/virtio/virtio_pci_common.h | 6 --
> > > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > > drivers/virtio/virtio_pci_modern.c | 6 --
> > > 4 files changed, 32 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c
> > > b/drivers/virtio/virtio_pci_common.c
> > > index b35bb2d57f62..0b9523e6dd39 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> > >"Force legacy mode for transitional virtio 1 devices");
> > > #endif
> > >
> > > -/* wait for pending irq handlers */
> > > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > > +/* disable irq handlers */
> > > +void vp_disable_vectors(struct virtio_device *vdev)
> > > {
> > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > int i;
> > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> > > synchronize_irq(vp_dev->pci_dev->irq);
> > >
> > > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > +}
> > > +
> > > +/* enable irq handlers */
> > > +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)
> > > + return;
> > > +
> > > + for (i = 0; i < vp_dev->msix_vectors; ++i)
> > > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > > }
> > >
> > > /* the notify function used when creating a virt queue */
> > > @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct
> > > virtio_device *vdev, int nvectors,
> > > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > >"%s-config", name);
> > > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > > - vp_config_changed, 0, vp_dev->msix_names[v],
> > > + vp_config_changed, IRQF_NO_AUTOEN,
> > > + vp_dev->msix_names[v],
> > > vp_dev);
> > > if (err)
> > > goto error;
> > > @@ -160,7 +174,8 @@ static int vp_request_msix_vectors(struct
> > > virtio_device *vdev, int nvectors,
> > > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> > >"%s-virtqueues", name);
> > > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > > - vp_vring_interrupt, 0,
> > > vp_dev->msix_names[v],
> > > + vp_vring_interrupt, IRQF_NO_AUTOEN,
> > > + vp_dev->msix_names[v],
> > > vp_dev);
> > > if (err)
> > > goto error;
> > > @@ -337,7 +352,7 @@ static int vp_find_vqs_msix(struct virtio_device
> > > *vdev, unsigned nvqs,
> > >"%s-%s",
> > >dev_name(_dev->vdev.dev), names[i]);
> > > err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
> > > - vring_interrupt, 0,
> > > +
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 2:04 PM Michael S. Tsirkin wrote:
>
> On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> > We used to synchronize pending MSI-X irq handlers via
> > synchronize_irq(), this may not work for the untrusted device which
> > may keep sending interrupts after reset which may lead unexpected
> > results. Similarly, we should not enable MSI-X interrupt until the
> > device is ready. So this patch fixes those two issues by:
> >
> > 1) switching to use disable_irq() to prevent the virtio interrupt
> >handlers to be called after the device is reset.
> > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
> >
> > This can make sure the virtio interrupt handler won't be called before
> > virtio_device_ready() and after reset.
> >
> > Signed-off-by: Jason Wang
>
> I don't get the threat model here. Isn't disabling irqs done by the
> hypervisor anyway? Is there a reason to trust disable_irq but not
> device reset?
My understanding is that e.g in the case of SEV/TDX we don't trust the
hypervisor. So the hypervisor can keep sending interrupts even if the
device is reset. The guest can only trust its own software interrupt
management logic to avoid call virtio callback in this case.
Thanks
>
> Cc a bunch more people ...
>
>
> > ---
> > drivers/virtio/virtio_pci_common.c | 27 +--
> > drivers/virtio/virtio_pci_common.h | 6 --
> > drivers/virtio/virtio_pci_legacy.c | 5 +++--
> > drivers/virtio/virtio_pci_modern.c | 6 --
> > 4 files changed, 32 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c
> > b/drivers/virtio/virtio_pci_common.c
> > index b35bb2d57f62..0b9523e6dd39 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
> >"Force legacy mode for transitional virtio 1 devices");
> > #endif
> >
> > -/* wait for pending irq handlers */
> > -void vp_synchronize_vectors(struct virtio_device *vdev)
> > +/* disable irq handlers */
> > +void vp_disable_vectors(struct virtio_device *vdev)
> > {
> > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > int i;
> > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> > synchronize_irq(vp_dev->pci_dev->irq);
> >
> > for (i = 0; i < vp_dev->msix_vectors; ++i)
> > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > +}
> > +
> > +/* enable irq handlers */
> > +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)
> > + return;
> > +
> > + for (i = 0; i < vp_dev->msix_vectors; ++i)
> > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> > }
> >
> > /* the notify function used when creating a virt queue */
> > @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct virtio_device
> > *vdev, int nvectors,
> > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> >"%s-config", name);
> > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > - vp_config_changed, 0, vp_dev->msix_names[v],
> > + vp_config_changed, IRQF_NO_AUTOEN,
> > + vp_dev->msix_names[v],
> > vp_dev);
> > if (err)
> > goto error;
> > @@ -160,7 +174,8 @@ static int vp_request_msix_vectors(struct virtio_device
> > *vdev, int nvectors,
> > snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
> >"%s-virtqueues", name);
> > err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> > - vp_vring_interrupt, 0,
> > vp_dev->msix_names[v],
> > + vp_vring_interrupt, IRQF_NO_AUTOEN,
> > + vp_dev->msix_names[v],
> > vp_dev);
> > if (err)
> > goto error;
> > @@ -337,7 +352,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
> > unsigned nvqs,
> >"%s-%s",
> >dev_name(_dev->vdev.dev), names[i]);
> > err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
> > - vring_interrupt, 0,
> > + vring_interrupt, IRQF_NO_AUTOEN,
> > vp_dev->msix_names[msix_vec],
> > vqs[i]);
> > if (err)
> > diff --git a/drivers/virtio/virtio_pci_common.h
> > b/drivers/virtio/virtio_pci_common.h
> > index beec047a8f8d..a235ce9ff6a5 100644
> > --- a/drivers/virtio/virtio_pci_common.h
> > +++ b/drivers/virtio/virtio_pci_common.h
> > @@ -102,8 +102,10
Re: [PATCH 6/9] virtio_pci: harden MSI-X interrupts
On Mon, Sep 13, 2021 at 01:53:50PM +0800, Jason Wang wrote:
> We used to synchronize pending MSI-X irq handlers via
> synchronize_irq(), this may not work for the untrusted device which
> may keep sending interrupts after reset which may lead unexpected
> results. Similarly, we should not enable MSI-X interrupt until the
> device is ready. So this patch fixes those two issues by:
>
> 1) switching to use disable_irq() to prevent the virtio interrupt
>handlers to be called after the device is reset.
> 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready()
>
> This can make sure the virtio interrupt handler won't be called before
> virtio_device_ready() and after reset.
>
> Signed-off-by: Jason Wang
I don't get the threat model here. Isn't disabling irqs done by the
hypervisor anyway? Is there a reason to trust disable_irq but not
device reset?
Cc a bunch more people ...
> ---
> drivers/virtio/virtio_pci_common.c | 27 +--
> drivers/virtio/virtio_pci_common.h | 6 --
> drivers/virtio/virtio_pci_legacy.c | 5 +++--
> drivers/virtio/virtio_pci_modern.c | 6 --
> 4 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index b35bb2d57f62..0b9523e6dd39 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy,
>"Force legacy mode for transitional virtio 1 devices");
> #endif
>
> -/* wait for pending irq handlers */
> -void vp_synchronize_vectors(struct virtio_device *vdev)
> +/* disable irq handlers */
> +void vp_disable_vectors(struct virtio_device *vdev)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> int i;
> @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev)
> synchronize_irq(vp_dev->pci_dev->irq);
>
> for (i = 0; i < vp_dev->msix_vectors; ++i)
> - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i));
> + disable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> +}
> +
> +/* enable irq handlers */
> +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)
> + return;
> +
> + for (i = 0; i < vp_dev->msix_vectors; ++i)
> + enable_irq(pci_irq_vector(vp_dev->pci_dev, i));
> }
>
> /* the notify function used when creating a virt queue */
> @@ -141,7 +154,8 @@ static int vp_request_msix_vectors(struct virtio_device
> *vdev, int nvectors,
> snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
>"%s-config", name);
> err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> - vp_config_changed, 0, vp_dev->msix_names[v],
> + vp_config_changed, IRQF_NO_AUTOEN,
> + vp_dev->msix_names[v],
> vp_dev);
> if (err)
> goto error;
> @@ -160,7 +174,8 @@ static int vp_request_msix_vectors(struct virtio_device
> *vdev, int nvectors,
> snprintf(vp_dev->msix_names[v], sizeof *vp_dev->msix_names,
>"%s-virtqueues", name);
> err = request_irq(pci_irq_vector(vp_dev->pci_dev, v),
> - vp_vring_interrupt, 0, vp_dev->msix_names[v],
> + vp_vring_interrupt, IRQF_NO_AUTOEN,
> + vp_dev->msix_names[v],
> vp_dev);
> if (err)
> goto error;
> @@ -337,7 +352,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
> unsigned nvqs,
>"%s-%s",
>dev_name(_dev->vdev.dev), names[i]);
> err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
> - vring_interrupt, 0,
> + vring_interrupt, IRQF_NO_AUTOEN,
> vp_dev->msix_names[msix_vec],
> vqs[i]);
> if (err)
> diff --git a/drivers/virtio/virtio_pci_common.h
> b/drivers/virtio/virtio_pci_common.h
> index beec047a8f8d..a235ce9ff6a5 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -102,8 +102,10 @@ static struct virtio_pci_device *to_vp_device(struct
> virtio_device *vdev)
> return container_of(vdev, struct virtio_pci_device, vdev);
> }
>
> -/* wait for pending irq handlers */
> -void vp_synchronize_vectors(struct virtio_device *vdev);
> +/* disable irq handlers */
> +void vp_disable_vectors(struct virtio_device *vdev);
> +/* enable irq handlers */
> +void vp_enable_vectors(struct virtio_device *vdev);
> /* the notify function used when creating a virt queue */
> bool vp_notify(struct virtqueue *vq);
> /* the
