From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkCm2-0007YV-TL for qemu-devel@nongnu.org; Wed, 14 Sep 2016 12:11:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bkClx-0005jJ-9o for qemu-devel@nongnu.org; Wed, 14 Sep 2016 12:10:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45728) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkClx-0005iz-1A for qemu-devel@nongnu.org; Wed, 14 Sep 2016 12:10:53 -0400 Date: Wed, 14 Sep 2016 19:10:50 +0300 From: "Michael S. Tsirkin" Message-ID: <20160914161050.mz4gdkwh7hxij3ky@redhat.com> References: <147377800565.11859.4411044563640180545.stgit@brijesh-build-machine> <147377820679.11859.11888810000954712438.stgit@brijesh-build-machine> <20160914052834-mutt-send-email-mst@kernel.org> <20160914120943.GB24695@thinpad.lan.raisama.net> <1bf1ae78-88de-316f-5d1f-00a5e2fc9fc0@redhat.com> <20160914160603-mutt-send-email-mst@kernel.org> <20160914135159.GC24695@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160914135159.GC24695@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [RFC PATCH v1 20/22] fw_cfg: sev: disable dma in real mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Paolo Bonzini , Brijesh Singh , crosthwaite.peter@gmail.com, armbru@redhat.com, p.fedin@samsung.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, rth@twiddle.net 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 > > > >>>> --- > > > >>>> 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