| Message ID | 147377820679.11859.11888810000954712438.stgit@brijesh-build-machine (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 13, 2016 at 10:50:06AM -0400, 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> Why do we need new interfaces for this though? Just set dma_enabled=false on command line. > --- > 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 */
Hi Michael, On 09/13/2016 01:39 PM, Michael S. Tsirkin wrote: > On Tue, Sep 13, 2016 at 10:50:06AM -0400, 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> > > Why do we need new interfaces for this though? Just > set dma_enabled=false on command line. > Thanks, i was not aware of dma_enabled=false command line option. Could you please tell me how to set this command line option? I see this in 'qemu --help' -fw_cfg [name=]<name>,file=<file> add named fw_cfg entry from file -fw_cfg [name=]<name>,string=<str> add named fw_cfg entry from string >> --- >> 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 */
On Tue, Sep 13, 2016 at 03:46:15PM -0500, Brijesh Singh wrote: > Hi Michael, > > On 09/13/2016 01:39 PM, Michael S. Tsirkin wrote: > > On Tue, Sep 13, 2016 at 10:50:06AM -0400, 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> > > > > Why do we need new interfaces for this though? Just > > set dma_enabled=false on command line. > > > > Thanks, i was not aware of dma_enabled=false command line option. > Could you please tell me how to set this command line option? > > I see this in 'qemu --help' > > -fw_cfg [name=]<name>,file=<file> > add named fw_cfg entry from file > -fw_cfg [name=]<name>,string=<str> > add named fw_cfg entry from string > For example, set it on all fw cfg devices: -global fw_cfg.dma_enabled=false should work. > > > --- > > > 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 */
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
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. Even assuming there are some guests that have secret data during boot, I would first upstream the main part of the feature for normal guests, then weight the extra security if any against the features and performance lost (like slower boot times).
On 14/09/2016 04:33, 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. > > Even assuming there are some guests that have secret data during boot, > I would first upstream the main part of the feature for normal guests, > then weight the extra security if any against the features and > performance lost (like slower boot times). If you can't trust boot, any encryption done afterwards is totally pointless. Paolo
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.
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
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. I suggest we just drop this attestation thing in v1. Try to merge something minimal that actually works first. 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.
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.
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 <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. 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
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.
On Wed, Sep 14, 2016 at 10:58:08AM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 04:33, 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. > > > > Even assuming there are some guests that have secret data during boot, > > I would first upstream the main part of the feature for normal guests, > > then weight the extra security if any against the features and > > performance lost (like slower boot times). > > If you can't trust boot, any encryption done afterwards is totally > pointless. > > Paolo You trust hypervisor anyway, it's all mitigation, being about making attacks harder, isn't it? So if the attack has to happen in the window when guest boots within BIOS, that might be good enough. Or just don't boot in the cloud, move guest there after boot.
On Wed, Sep 14, 2016 at 09:09:43AM -0300, 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. Again it isn't clear how much value does attestation have, we are assuming arbitrary restrictions on the attacker such as inability to trigger exits at random times, why not assume it can't attack guest during boot? IOW it seems reasonable to just ignore the need for attestation completely as the first step. Get the other stuff merged first. > -- > Eduardo
Hi Michael, > > Again it isn't clear how much value does attestation have, > we are assuming arbitrary restrictions on the attacker such > as inability to trigger exits at random times, why not > assume it can't attack guest during boot? > IOW it seems reasonable to just ignore the need for attestation > completely as the first step. > Get the other stuff merged first. > Thanks for feedbacks. In v2, I will try to remove the attestation code and we can revisit it later. -Brijesh
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 */
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(+)