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

[Michael S. Tsirkin]

On Wed, Sep 14, 2016 at 10:51:59AM -0300, Eduardo Habkost wrote:
> 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 <address@hidden>
> > > >>>> ---
> > > >>>>  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.

You seem to argue that this patch does protect against malicious
hypervisors. Is this so?

> > 
> > 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?

This is what the cover letter says: protecting against passive
adversaries: if the adversary can read all hypervisor memory but nothing
else, you can stop some information leaks to that adversary.


> It sounds like adding dead code that nobody
> would use until attestation is done properly.

All I am saying is let's assume guests will ignore the measurement
result for now.  Judging by e.g. the whitepaper that I read,
attestation is designed to protect against a malicious hypervisor, so
it's part of a future vision, not the current patchset.



> > 
> > 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