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.si...@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
