Re: [Qemu-devel] [RFC PATCH v1 20/22] fw_cfg: sev: disable dma in real mode

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, Sep 14, 2016 at 04:14:58PM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 14, 2016 at 03:01:51PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 14/09/2016 14:09, Eduardo Habkost wrote:
> > > On Wed, Sep 14, 2016 at 05:33:09AM +0300, Michael S. Tsirkin wrote:
> > >> On Wed, Sep 14, 2016 at 12:53:36AM +0200, Paolo Bonzini wrote:
> > >>>
> > >>>
> > >>> On 13/09/2016 16:50, Brijesh Singh wrote:
> > >>>> In SEV-enabled guest dma should be performed on shared pages. Since
> > >>>> the SeaBIOS executes in non PAE mode and does not have access to C-bit
> > >>>> to create a shared page hence disable the dma operation when reading
> > >>>> from fw_cfg interface.
> > >>>>
> > >>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > >>>> ---
> > >>>>  hw/nvram/fw_cfg.c |    6 ++++++
> > >>>>  1 file changed, 6 insertions(+)
> > >>>>
> > >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > >>>> index 6a68e59..aca99e9 100644
> > >>>> --- a/hw/nvram/fw_cfg.c
> > >>>> +++ b/hw/nvram/fw_cfg.c
> > >>>> @@ -24,6 +24,7 @@
> > >>>>  #include "qemu/osdep.h"
> > >>>>  #include "hw/hw.h"
> > >>>>  #include "sysemu/sysemu.h"
> > >>>> +#include "sysemu/kvm.h"
> > >>>>  #include "sysemu/dma.h"
> > >>>>  #include "hw/boards.h"
> > >>>>  #include "hw/isa/isa.h"
> > >>>> @@ -1009,6 +1010,11 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
> > >>>>      FWCfgIoState *s = FW_CFG_IO(dev);
> > >>>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > >>>>  
> > >>>> +    /* disable dma on fw_cfg when SEV is enabled */
> > >>>> +    if (kvm_sev_enabled()) {
> > >>>> +        qdev_prop_set_bit(dev, "dma_enabled", false);
> > >>>> +    }
> > >>>> +
> > >>>>      /* when using port i/o, the 8-bit data register ALWAYS overlaps
> > >>>>       * with half of the 16-bit control register. Hence, the total size
> > >>>>       * of the i/o region used is FW_CFG_CTL_SIZE */
> > >>>>
> > >>>>
> > >>>>
> > >>>
> > >>> As Michael said, a workaround is possible using -global.  However, I
> > >>> really think that SEV should be implemented in UEFI firmware.  Because
> > >>> it runs in 64-bit mode, it would be able to run in paged mode and it
> > >>> would not have to encrypt everything before the SEV launch command.
> > >>>
> > >>> For example secure boot can be used to authenticate the kernel against
> > >>> keys provided by the owner in encrypted flash, or GRUB2 can be placed in
> > >>> the firmware by the owner and used to boot from a LUKS-encrypted /boot
> > >>> partition.
> > >>>
> > >>> Paolo
> > >>
> > >> Frankly I don't understand why do you need to mess with boot at all.
> > >> Quoting the cover letter:
> > >>
> > >> 	SEV is designed to protect guest VMs from a benign but vulnerable
> > >> 	(i.e. not fully malicious) hypervisor. In particular, it reduces the
> > >> 	attack
> > >> 	surface of guest VMs and can prevent certain types of VM-escape bugs
> > >> 	(e.g. hypervisor read-anywhere) from being used to steal guest data.
> > >>
> > >> it seems highly unlikely that any secret data is used during boot.
> > >> So just let guest boot normally, and encrypt afterwards.
> > > 
> > > After boot seems too late for the attestation part (see Section
> > > 1.3.1: Launch in the spec), unless you can ensure the memory
> > > contents will always be exactly what the guest owner expects
> > > after every boot.
> > 
> > And the attestation is what lets the guest check that the memory
> > contents are indeed what the guest owner expects.
> > 
> > Paolo
> 
> So the cover letter says hypervisor is benign, and then people turn
> around and start discussing guest owner checking memory as if hypervisor
> is malicious and might load something unexpected there.  Makes no sense
> to me.

Cover letter says "a benign but vulnerable (i.e. not fully
malicious) hypervisor". The hypervisor might be compromised from
the very beginning, but even a compromised hypervisor shouldn't
be able to provide an attestation that it has encrypted the
memory.

> 
> I suggest we just drop this attestation thing in v1. Try to merge
> something minimal that actually works first.

As far as I can see from the spec, attestation is part of the
encryption process (the Launch event). I don't see how this could
be even dropped.

One may argue to drop the usefulness of the attestation by doing
it very late. But I don't really see the point of doing it: are
there any users that would want to use SEV with a useless
attestation process? It sounds like adding dead code that nobody
would use until attestation is done properly.

> 
> You can always extend this to add more features later:
> whether there's any value in guest checking its own memory
> is something that would need a separate discussion at
> a higher level. I'm not saying there isn't.
> Let's do one thing at a time though.

-- 
Eduardo