On Tue, Apr 12, 2022 at 8:02 AM Halil Pasic <pa...@linux.ibm.com> wrote:
>
> On Mon, 11 Apr 2022 16:27:41 +0200
> Cornelia Huck <coh...@redhat.com> wrote:
>
> > On Mon, Apr 11 2022, Jason Wang <jasow...@redhat.com> wrote:
> >
> > > On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <m...@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 <coh...@redhat.com> wrote:
> > >> >
> > >> > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <m...@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 <t...@linutronix.de>
> > >> > > >> Cc: Peter Zijlstra <pet...@infradead.org>
> > >> > > >> Cc: "Paul E. McKenney" <paul...@kernel.org>
> > >> > > >> Cc: Marc Zyngier <m...@kernel.org>
> > >> > > >> Signed-off-by: Jason Wang <jasow...@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?
Exactly.
>
> 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.
Does s390 have something like memory encryption? (I guess yes). In the
case of x86 VM encryption, the I/O buffers were now done via software
IOTLB, that's why hardening of the virtio driver is needed to prevent
the hypervisor to poke the swiotlb etc.
Thanks
>
> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
