Re: [for-5.2 v4 10/10] s390: Recognize host-trust-limitation option

From: Halil Pasic <pasic@linux.ibm.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: "Cornelia Huck" <cohuck@redhat.com>,
	dgilbert@redhat.com, frankja@linux.ibm.com, pair@us.ibm.com,
	qemu-devel@nongnu.org, pbonzini@redhat.com,
	brijesh.singh@amd.com, ehabkost@redhat.com,
	marcel.apfelbaum@gmail.com, "Michael S. Tsirkin" <mst@redhat.com>,
	qemu-ppc@nongnu.org, kvm@vger.kernel.org, qemu-s390x@nongnu.org,
	"David Hildenbrand" <david@redhat.com>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	"Richard Henderson" <rth@twiddle.net>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	mdroth@linux.vnet.ibm.com, "Thomas Huth" <thuth@redhat.com>
Subject: Re: [for-5.2 v4 10/10] s390: Recognize host-trust-limitation option
Date: Fri, 11 Sep 2020 14:45:56 +0200	[thread overview]
Message-ID: <20200911144556.144ef065.pasic@linux.ibm.com> (raw)
In-Reply-To: <20200911000718.GF66834@yekko.fritz.box>

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

On Fri, 11 Sep 2020 10:07:18 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Sep 10, 2020 at 08:29:24PM +0200, Halil Pasic wrote:
> > On Thu, 10 Sep 2020 13:36:09 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Mon, 7 Sep 2020 17:22:53 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > 
> > > > On Fri, 24 Jul 2020 12:57:44 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > > At least some s390 cpu models support "Protected Virtualization" (PV),
> > > > > a mechanism to protect guests from eavesdropping by a compromised
> > > > > hypervisor.
> > > > > 
> > > > > This is similar in function to other mechanisms like AMD's SEV and
> > > > > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > > > > machine option.  s390 is a slightly special case, because we already
> > > > > supported PV, simply by using a CPU model with the required feature
> > > > > (S390_FEAT_UNPACK).
> > > > > 
> > > > > To integrate this with the option used by other platforms, we
> > > > > implement the following compromise:
> > > > > 
> > > > >  - When the host-trust-limitation option is set, s390 will recognize
> > > > >    it, verify that the CPU can support PV (failing if not) and set
> > > > >    virtio default options necessary for encrypted or protected guests,
> > > > >    as on other platforms.  i.e. if host-trust-limitation is set, we
> > > > >    will either create a guest capable of entering PV mode, or fail
> > > > >    outright  
> > > > 
> > > > Shouldn't we also fail outright if the virtio features are not PV
> > > > compatible (invalid configuration)?
> > > > 
> > > > I would like to see something like follows as a part of this series.
> > > > ----------------------------8<--------------------------
> > > > From: Halil Pasic <pasic@linux.ibm.com>
> > > > Date: Mon, 7 Sep 2020 15:00:17 +0200
> > > > Subject: [PATCH] virtio: handle host trust limitation
> > > > 
> > > > If host_trust_limitation_enabled() returns true, then emulated virtio
> > > > devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
> > > > capable of accessing all of the guest memory. Otherwise we are in
> > > > violation of the virtio specification.
> > > > 
> > > > Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
> > > > obligatory but missing.
> > > > 
> > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 5bd2a2f621..19b4b0a37a 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "hw/virtio/virtio-access.h"
> > > >  #include "sysemu/dma.h"
> > > >  #include "sysemu/runstate.h"
> > > > +#include "exec/host-trust-limitation.h"
> > > >  
> > > >  /*
> > > >   * The alignment to use between consumer and producer parts of vring.
> > > > @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
> > > >      /* Devices should either use vmsd or the load/save methods */
> > > >      assert(!vdc->vmsd || !vdc->load);
> > > >  
> > > > +    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
> > > > +        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > > +        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
> > > > +        error_propagate(errp, err);
> > > > +        return;
> > > 
> > > How can we get here? I assume only if the user explicitly turned the
> > > feature off while turning HTL on, as otherwise patch 9 should have
> > > taken care of it?
> > > 
> > 
> > Yes, we can get here only if iommu_platform is explicitly turned off.
> 
> Right.. my assumption was that if you really want to specify
> contradictory options, you get to keep both pieces.  Or, more
> seriously, there might be some weird experimental cases where this
> combination could do something useful if you really know what you're
> doing, and explicitly telling qemu to do this implies you know what
> you're doing.
> 

According to Michael, the correctness of a hypervisor is depending on
this (if device has restricted access to guest memory, but does not
present F_ACCESS_PLATFORM then the hypervisor is buggy).

Also a hotplug of such a misconfigured device is at the moment likely
bring down the guest on s390x.

The only scenario in which the guest has protected memory and the device
is able to access it, is a trusted HW device. But then we would need 
F_ACCESS_PLATFORM because it is a HW device. So I consider this combination
doing something useful very unlikely. Does anybody have a scenario where
this combination is legit and useful?

If such a scenario emerges later, I think the check can be refined or
dropped.

Regards,
Halil

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]