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

[Eduardo Habkost]

On Wed, Sep 14, 2016 at 07:10:50PM +0300, Michael S. Tsirkin wrote:
[...]
> > > > >> 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 am just assuming that the whole attestation system is there to
protect against compromised hypervisors.

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

I believe it tries to protect against additional attacks. To be
sure about it, I need to see the talk and read the specs more
carefully. (Maybe it's easier to simply wait for the patchset
author describe their thread model.)

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

I'm not sure about "future vision, not the current patchset"
part. I expect the current patchset to be effective against
additional attacks, but I agree this need to be more clearly
described in the patchset description/justification.

-- 
Eduardo