Cornelia Huck
2022-Apr-11 14:27 UTC
[PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Mon, Apr 11 2022, Jason Wang <jasowang at redhat.com> wrote:> On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst at redhat.com> wrote: >> >> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote: >> > On Wed, 06 Apr 2022 15:04:32 +0200 >> > Cornelia Huck <cohuck at redhat.com> wrote: >> > >> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst at redhat.com> wrote: >> > > >> > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote: >> > > >> This patch implements PCI version of synchronize_vqs(). >> > > >> >> > > >> Cc: Thomas Gleixner <tglx at linutronix.de> >> > > >> Cc: Peter Zijlstra <peterz at infradead.org> >> > > >> Cc: "Paul E. McKenney" <paulmck at kernel.org> >> > > >> Cc: Marc Zyngier <maz at kernel.org> >> > > >> Signed-off-by: Jason Wang <jasowang at redhat.com> >> > > > >> > > > Please add implementations at least for ccw and mmio. >> > > >> > > I'm not sure what (if anything) can/should be done for ccw... >> > >> > If nothing needs to be done I would like to have at least a comment in >> > the code that explains why. So that somebody who reads the code >> > doesn't wonder: why is virtio-ccw not implementing that callback. >> >> Right. >> >> I am currently thinking instead of making this optional in the >> core we should make it mandatory, and have transports which do not >> need to sync have an empty stub with documentation explaining why.Yes, that makes sense to me. If we can explain why we don't need to do anything, we should keep that explanation easily accessible.>> >> Also, do we want to document this sync is explicitly for irq enable/disable? >> synchronize_irq_enable_disable? > > I would not since the transport is not guaranteed to use an interrupt > for callbacks. > >> >> >> > > >> > > > >> > > >> --- >> > > >> drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++ >> > > >> drivers/virtio/virtio_pci_common.h | 2 ++ >> > > >> drivers/virtio/virtio_pci_legacy.c | 1 + >> > > >> drivers/virtio/virtio_pci_modern.c | 2 ++ >> > > >> 4 files changed, 19 insertions(+) >> > > >> >> > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c >> > > >> index d724f676608b..b78c8bc93a97 100644 >> > > >> --- a/drivers/virtio/virtio_pci_common.c >> > > >> +++ b/drivers/virtio/virtio_pci_common.c >> > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev) >> > > >> synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); >> > > >> } >> > > >> >> > > >> +void vp_synchronize_vqs(struct virtio_device *vdev) >> > > >> +{ >> > > >> + struct virtio_pci_device *vp_dev = to_vp_device(vdev); >> > > >> + int i; >> > > >> + >> > > >> + if (vp_dev->intx_enabled) { >> > > >> + synchronize_irq(vp_dev->pci_dev->irq); >> > > >> + return; >> > > >> + } >> > > >> + >> > > >> + for (i = 0; i < vp_dev->msix_vectors; ++i) >> > > >> + synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); >> > > >> +} >> > > >> + >> > > >> > > ...given that this seems to synchronize threaded interrupt handlers? >> > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one >> > > 'irq' for channel devices anyway, and the handler just calls the >> > > relevant callbacks directly.) >> > >> > Sorry I don't understand enough yet. A more verbose documentation on >> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would >> > surely benefit me. It may be more than enough for a back-belt but it >> > ain't enough for me to tell what is the callback supposed to accomplish.+1 for more explanations.>> > >> > I will have to study this discussion and the code more thoroughly. >> > Tentatively I side with Jason and Michael in a sense, that I don't >> > believe virtio-ccw is safe against rough interrupts. > > That's my feeling as well.I'd say ccw is safe against "notification interrupts before indicators have been registered". For the reverse case, maybe we should always invalidate the indicators in the reset case? More information regarding the attack vector would help here :) My main concern is that we would need to synchronize against a single interrupt that covers all kinds of I/O interrupts, not just a single device...
On Mon, 11 Apr 2022 16:27:41 +0200 Cornelia Huck <cohuck at redhat.com> wrote:> On Mon, Apr 11 2022, Jason Wang <jasowang at redhat.com> wrote: > > > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst at redhat.com> wrote: > >> > >> On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote: > >> > On Wed, 06 Apr 2022 15:04:32 +0200 > >> > Cornelia Huck <cohuck at redhat.com> wrote: > >> > > >> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst at redhat.com> wrote: > >> > > > >> > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote: > >> > > >> This patch implements PCI version of synchronize_vqs(). > >> > > >> > >> > > >> Cc: Thomas Gleixner <tglx at linutronix.de> > >> > > >> Cc: Peter Zijlstra <peterz at infradead.org> > >> > > >> Cc: "Paul E. McKenney" <paulmck at kernel.org> > >> > > >> Cc: Marc Zyngier <maz at kernel.org> > >> > > >> Signed-off-by: Jason Wang <jasowang at redhat.com> > >> > > > > >> > > > Please add implementations at least for ccw and mmio. > >> > > > >> > > I'm not sure what (if anything) can/should be done for ccw... > >> > > >> > If nothing needs to be done I would like to have at least a comment in > >> > the code that explains why. So that somebody who reads the code > >> > doesn't wonder: why is virtio-ccw not implementing that callback. > >> > >> Right. > >> > >> I am currently thinking instead of making this optional in the > >> core we should make it mandatory, and have transports which do not > >> need to sync have an empty stub with documentation explaining why. > > Yes, that makes sense to me. If we can explain why we don't need to do > anything, we should keep that explanation easily accessible. > > >> > >> Also, do we want to document this sync is explicitly for irq enable/disable? > >> synchronize_irq_enable_disable? > > > > I would not since the transport is not guaranteed to use an interrupt > > for callbacks. > > > >> > >> > >> > > > >> > > > > >> > > >> --- > >> > > >> drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++ > >> > > >> drivers/virtio/virtio_pci_common.h | 2 ++ > >> > > >> drivers/virtio/virtio_pci_legacy.c | 1 + > >> > > >> drivers/virtio/virtio_pci_modern.c | 2 ++ > >> > > >> 4 files changed, 19 insertions(+) > >> > > >> > >> > > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > >> > > >> index d724f676608b..b78c8bc93a97 100644 > >> > > >> --- a/drivers/virtio/virtio_pci_common.c > >> > > >> +++ b/drivers/virtio/virtio_pci_common.c > >> > > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev) > >> > > >> synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > >> > > >> } > >> > > >> > >> > > >> +void vp_synchronize_vqs(struct virtio_device *vdev) > >> > > >> +{ > >> > > >> + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > >> > > >> + int i; > >> > > >> + > >> > > >> + if (vp_dev->intx_enabled) { > >> > > >> + synchronize_irq(vp_dev->pci_dev->irq); > >> > > >> + return; > >> > > >> + } > >> > > >> + > >> > > >> + for (i = 0; i < vp_dev->msix_vectors; ++i) > >> > > >> + synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > >> > > >> +} > >> > > >> + > >> > > > >> > > ...given that this seems to synchronize threaded interrupt handlers? > >> > > Halil, do you think ccw needs to do anything? (AFAICS, we only have one > >> > > 'irq' for channel devices anyway, and the handler just calls the > >> > > relevant callbacks directly.) > >> > > >> > Sorry I don't understand enough yet. A more verbose documentation on > >> > "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would > >> > surely benefit me. It may be more than enough for a back-belt but it > >> > ain't enough for me to tell what is the callback supposed to accomplish. > > +1 for more explanations. > > >> > > >> > I will have to study this discussion and the code more thoroughly. > >> > Tentatively I side with Jason and Michael in a sense, that I don't > >> > believe virtio-ccw is safe against rough interrupts. > > > > That's my feeling as well. > > I'd say ccw is safe against "notification interrupts before indicators > have been registered".I believe Jason's scope is broader than that. Let me try to explain. A quote form the standard: """ 3.1.1 Driver Requirements: Device Initialization The driver MUST follow this sequence to initialize a device: 1. Reset the device. 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device. 3. Set the DRIVER status bit: the guest OS knows how to drive the device. 4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it. 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step. 6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not support our subset of features and the device is unusable. 7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup, reading and possibly writing the device?s virtio configuration space, and population of virtqueues. 8. Set the DRIVER_OK status bit. At this point the device is ?live?. """ That means stuff may happen between "discovery of virtqueues" and "DRIVER_OK". So it is not sufficient to be "safe against notifications before indicators have been registered", but we want to be also safe between "indicators have been registered" and "DRIVER_OK status has been set". @Jason: can you confirm? Regarding the question "are we safe against notifications before indicators have been registered" I think we really need to think about something like Secure Execution. We don't have, and we are unlikely to have in hardware virtio-ccw implementations, and for a malicious hypervisor that has full access to the guest memory hardening makes no sense. But if we assume that an attacker can inject adapter interrupts for an arbitrary ISC, and can poke any shared memory (notifier bits are shared)... Things might become critical already when register_adapter_interrupt() does it's magic.> For the reverse case, maybe we should always > invalidate the indicators in the reset case? More information regarding > the attack vector would help here :) > > My main concern is that we would need to synchronize against a single > interrupt that covers all kinds of I/O interrupts, not just a single > device... >Could we synchronize on struct airq_info's lock member? If we were to grab all of these that might be involved... AFAIU for the synchronize implementation we need a lock or a set of locks that contain all the possible vring_interrupt() calls with the queuues that belong to the given device as a critical section. That way, one has the acquire's and release's in place so that the vrign_interrupt() either guaranteed to finish before the change of driver_ready is guaranteed to be complete, or it is guaranteed to see the change. In any case, I guess we should first get clear on the first part. I.e. when do we want to allow host->guest notifications. Regards, Halil