| Message ID | 147377810767.11859.4668503556528840901.stgit@brijesh-build-machine |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 13, 2016 at 10:48:27AM -0400, Brijesh Singh wrote: > The SEV DEBUG_DECRYPT command is used for decrypting a guest memory > for the debugging purposes. Note that debugging is permitting only > when guest policy allows it. When wouldn't you want to allow it? I don't see value in a "break debugging" feature. > For more information see [1], section 7.1 > > [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf Please add comments documenting APIs. Spec links to figure out implementation is one thing, but you really can't require people to read specs just to figure out how to use an API. > The following KVM RFC patches defines and implements this command > > http://marc.info/?l=kvm&m=147190852423972&w=2 > http://marc.info/?l=kvm&m=147191068524579&w=2 > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > include/sysemu/sev.h | 10 ++++++++++ > sev.c | 23 +++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h > index ab03c5d..5872c3e 100644 > --- a/include/sysemu/sev.h > +++ b/include/sysemu/sev.h > @@ -55,4 +55,14 @@ int kvm_sev_guest_finish(void); > */ > int kvm_sev_guest_measurement(uint8_t *measurement); > > +/** > + * kvm_sev_dbg_decrypt - decrypt the guest memory for debugging purposes > + * @src - guest memory address > + * @dest - host memory address where the decrypted data should be copied > + * @length - length of memory region > + * > + * Returns: 0 on success and dest will contains the decrypted data > + */ > +int kvm_sev_dbg_decrypt(uint8_t *dest, const uint8_t *src, uint32_t len); > + > #endif > diff --git a/sev.c b/sev.c > index 055ed83..c7031d3 100644 > --- a/sev.c > +++ b/sev.c > @@ -432,3 +432,26 @@ int kvm_sev_guest_measurement(uint8_t *out) > > return 0; > } > + > +int kvm_sev_dbg_decrypt(uint8_t *dst, const uint8_t *src, uint32_t len) > +{ > + int ret; > + struct kvm_sev_dbg_decrypt decrypt; > + struct kvm_sev_issue_cmd input; > + > + decrypt.src_addr = (unsigned long)src; > + decrypt.dst_addr = (unsigned long)dst; > + decrypt.length = len; > + > + input.cmd = KVM_SEV_DBG_DECRYPT; > + input.opaque = (unsigned long)&decrypt; > + ret = kvm_vm_ioctl(kvm_state, KVM_SEV_ISSUE_CMD, &input); > + if (ret) { > + fprintf(stderr, "SEV: dbg_decrypt failed ret=%d(%#010x)\n", > + ret, input.ret_code); > + return 1; > + } > + > + DPRINTF("SEV: DBG_DECRYPT dst %p src %p sz %d\n", dst, src, len); > + return 0; > +}
On 14/09/2016 04:28, Michael S. Tsirkin wrote: > On Tue, Sep 13, 2016 at 10:48:27AM -0400, Brijesh Singh wrote: >> The SEV DEBUG_DECRYPT command is used for decrypting a guest memory >> for the debugging purposes. Note that debugging is permitting only >> when guest policy allows it. > > When wouldn't you want to allow it? > I don't see value in a "break debugging" feature. One man's "allow debugging" feature is another man's "break encryption" feature. :) Paolo > >> For more information see [1], section 7.1 >> >> [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf > > Please add comments documenting APIs. Spec links to figure out > implementation is one thing, but you really can't require people > to read specs just to figure out how to use an API. > >> The following KVM RFC patches defines and implements this command >> >> http://marc.info/?l=kvm&m=147190852423972&w=2 >> http://marc.info/?l=kvm&m=147191068524579&w=2 >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> include/sysemu/sev.h | 10 ++++++++++ >> sev.c | 23 +++++++++++++++++++++++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h >> index ab03c5d..5872c3e 100644 >> --- a/include/sysemu/sev.h >> +++ b/include/sysemu/sev.h >> @@ -55,4 +55,14 @@ int kvm_sev_guest_finish(void); >> */ >> int kvm_sev_guest_measurement(uint8_t *measurement); >> >> +/** >> + * kvm_sev_dbg_decrypt - decrypt the guest memory for debugging purposes >> + * @src - guest memory address >> + * @dest - host memory address where the decrypted data should be copied >> + * @length - length of memory region >> + * >> + * Returns: 0 on success and dest will contains the decrypted data >> + */ >> +int kvm_sev_dbg_decrypt(uint8_t *dest, const uint8_t *src, uint32_t len); >> + >> #endif >> diff --git a/sev.c b/sev.c >> index 055ed83..c7031d3 100644 >> --- a/sev.c >> +++ b/sev.c >> @@ -432,3 +432,26 @@ int kvm_sev_guest_measurement(uint8_t *out) >> >> return 0; >> } >> + >> +int kvm_sev_dbg_decrypt(uint8_t *dst, const uint8_t *src, uint32_t len) >> +{ >> + int ret; >> + struct kvm_sev_dbg_decrypt decrypt; >> + struct kvm_sev_issue_cmd input; >> + >> + decrypt.src_addr = (unsigned long)src; >> + decrypt.dst_addr = (unsigned long)dst; >> + decrypt.length = len; >> + >> + input.cmd = KVM_SEV_DBG_DECRYPT; >> + input.opaque = (unsigned long)&decrypt; >> + ret = kvm_vm_ioctl(kvm_state, KVM_SEV_ISSUE_CMD, &input); >> + if (ret) { >> + fprintf(stderr, "SEV: dbg_decrypt failed ret=%d(%#010x)\n", >> + ret, input.ret_code); >> + return 1; >> + } >> + >> + DPRINTF("SEV: DBG_DECRYPT dst %p src %p sz %d\n", dst, src, len); >> + return 0; >> +} > >
On Wed, Sep 14, 2016 at 10:57:34AM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 04:28, Michael S. Tsirkin wrote: > > On Tue, Sep 13, 2016 at 10:48:27AM -0400, Brijesh Singh wrote: > >> The SEV DEBUG_DECRYPT command is used for decrypting a guest memory > >> for the debugging purposes. Note that debugging is permitting only > >> when guest policy allows it. > > > > When wouldn't you want to allow it? > > I don't see value in a "break debugging" feature. > > One man's "allow debugging" feature is another man's "break encryption" > feature. :) > > Paolo Does this break anything? If so this needs better documentation. Again, don't assume users will read specs. If the flag is called "allow debug" then it is reasonable to assume users will use it exactly to allow debug. I assumed that with debug on, memory is still encrypted but the hypervisor can break encryption, and as the cover letter states, the hypervisor is assumed benign. If true I don't see a need to give users more rope. > > > >> For more information see [1], section 7.1 > >> > >> [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf > > > > Please add comments documenting APIs. Spec links to figure out > > implementation is one thing, but you really can't require people > > to read specs just to figure out how to use an API. > > > >> The following KVM RFC patches defines and implements this command > >> > >> http://marc.info/?l=kvm&m=147190852423972&w=2 > >> http://marc.info/?l=kvm&m=147191068524579&w=2 > >> > >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >> --- > >> include/sysemu/sev.h | 10 ++++++++++ > >> sev.c | 23 +++++++++++++++++++++++ > >> 2 files changed, 33 insertions(+) > >> > >> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h > >> index ab03c5d..5872c3e 100644 > >> --- a/include/sysemu/sev.h > >> +++ b/include/sysemu/sev.h > >> @@ -55,4 +55,14 @@ int kvm_sev_guest_finish(void); > >> */ > >> int kvm_sev_guest_measurement(uint8_t *measurement); > >> > >> +/** > >> + * kvm_sev_dbg_decrypt - decrypt the guest memory for debugging purposes > >> + * @src - guest memory address > >> + * @dest - host memory address where the decrypted data should be copied > >> + * @length - length of memory region > >> + * > >> + * Returns: 0 on success and dest will contains the decrypted data > >> + */ > >> +int kvm_sev_dbg_decrypt(uint8_t *dest, const uint8_t *src, uint32_t len); > >> + > >> #endif > >> diff --git a/sev.c b/sev.c > >> index 055ed83..c7031d3 100644 > >> --- a/sev.c > >> +++ b/sev.c > >> @@ -432,3 +432,26 @@ int kvm_sev_guest_measurement(uint8_t *out) > >> > >> return 0; > >> } > >> + > >> +int kvm_sev_dbg_decrypt(uint8_t *dst, const uint8_t *src, uint32_t len) > >> +{ > >> + int ret; > >> + struct kvm_sev_dbg_decrypt decrypt; > >> + struct kvm_sev_issue_cmd input; > >> + > >> + decrypt.src_addr = (unsigned long)src; > >> + decrypt.dst_addr = (unsigned long)dst; > >> + decrypt.length = len; > >> + > >> + input.cmd = KVM_SEV_DBG_DECRYPT; > >> + input.opaque = (unsigned long)&decrypt; > >> + ret = kvm_vm_ioctl(kvm_state, KVM_SEV_ISSUE_CMD, &input); > >> + if (ret) { > >> + fprintf(stderr, "SEV: dbg_decrypt failed ret=%d(%#010x)\n", > >> + ret, input.ret_code); > >> + return 1; > >> + } > >> + > >> + DPRINTF("SEV: DBG_DECRYPT dst %p src %p sz %d\n", dst, src, len); > >> + return 0; > >> +} > > > >
On 14/09/2016 15:05, Michael S. Tsirkin wrote: > I assumed that with debug on, memory is still encrypted but the > hypervisor can break encryption, and as the cover letter states, the > hypervisor is assumed benign. If true I don't see a need to > give users more rope. The hypervisor is assumed benign but vulnerable. So, if somebody breaks the hypervisor, you would like to make it as hard as possible for the attacker to do evil stuff to the guests. If the attacker can just ask the secure processor "decrypt some memory for me", then the encryption is effectively broken. Paolo
On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > I assumed that with debug on, memory is still encrypted but the > > hypervisor can break encryption, and as the cover letter states, the > > hypervisor is assumed benign. If true I don't see a need to > > give users more rope. > > The hypervisor is assumed benign but vulnerable. > > So, if somebody breaks the hypervisor, you would like to make it as hard > as possible for the attacker to do evil stuff to the guests. If the > attacker can just ask the secure processor "decrypt some memory for me", > then the encryption is effectively broken. So there's going to be a tradeoff here between use of SEV and use of certain other features. eg, it seems that if you're using SEV, then any concept of creating & analysing guest core dumps from the host is out. It seems that SEV on its own is insufficient - there is at least some interaction with storage. eg merely running a guest with SEV is not going to guarantee security if the guest OS is able to swap out to a non-encrypted disk. You could run LUKS inside the guest but that has a number of downsides. How to provide the decryption key for LUKS at startup without guest admin interaction. Then there is the issue that if you take snapshots of the guest disk in the host, this is weakening the security of LUKS, since you're keeping around copies of the same logical guest sector with different contents which allows for improve crytoanalysis. These are reasons for using LUKS on the host instead of in the guest, but then the decryption kjeys for LUKS are in the QEMU process in memory which is (IIUC) not going to be protected by SEV ? Unles there's a way for QEMU to do allocations which are SEV protected for its own purposes ? Regards, Daniel
On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > I assumed that with debug on, memory is still encrypted but the > > hypervisor can break encryption, and as the cover letter states, the > > hypervisor is assumed benign. If true I don't see a need to > > give users more rope. > > The hypervisor is assumed benign but vulnerable. Vulnerable to information leaks, yes. > So, if somebody breaks the hypervisor, you would like to make it as hard > as possible We don't just do this at random. Need some proof it's actually making things harder. > for the attacker to do evil stuff to the guests. Break as in make it do things? This is a possible model, but this is not what the cover letter states. As far as I can tell, encrypting memory does not protect against an attacker that can execute code in the hypervisor, if only for the reason that a lot of guest info is not in memory as CPU always accesses memory through registers. > If the > attacker can just ask the secure processor "decrypt some memory for me", > then the encryption is effectively broken. > > Paolo Not at all, if all you have is hypervisor read-anywhere access, then that is not broken. This seems to be the threat model that the patchset targets, again based on the cover letter.
On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > I assumed that with debug on, memory is still encrypted but the > > > hypervisor can break encryption, and as the cover letter states, the > > > hypervisor is assumed benign. If true I don't see a need to > > > give users more rope. > > > > The hypervisor is assumed benign but vulnerable. > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > as possible for the attacker to do evil stuff to the guests. If the > > attacker can just ask the secure processor "decrypt some memory for me", > > then the encryption is effectively broken. > > So there's going to be a tradeoff here between use of SEV and use of > certain other features. eg, it seems that if you're using SEV, then > any concept of creating & analysing guest core dumps from the host > is out. I don't see why - as long as we don't trigger dumps, there's no leak :) > It seems that SEV on its own is insufficient - there is at least some > interaction with storage. eg merely running a guest with SEV is not > going to guarantee security if the guest OS is able to swap out to a > non-encrypted disk. You could run LUKS inside the guest but that has > a number of downsides. How to provide the decryption key for LUKS > at startup without guest admin interaction. Then there is the issue > that if you take snapshots of the guest disk in the host, this is > weakening the security of LUKS, since you're keeping around copies > of the same logical guest sector with different contents which > allows for improve crytoanalysis. These are reasons for using LUKS > on the host instead of in the guest, but then the decryption kjeys > for LUKS are in the QEMU process in memory which is (IIUC) not going > to be protected by SEV ? Unles there's a way for QEMU to do allocations > which are SEV protected for its own purposes ? > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/13/2016 09:28 PM, Michael S. Tsirkin wrote: > On Tue, Sep 13, 2016 at 10:48:27AM -0400, Brijesh Singh wrote: >> The SEV DEBUG_DECRYPT command is used for decrypting a guest memory >> for the debugging purposes. Note that debugging is permitting only >> when guest policy allows it. > > When wouldn't you want to allow it? > I don't see value in a "break debugging" feature. > A guest owner needs to provide the launch parameters before we launch a SEV guest, a typical input parameters looks like this. [sev-launch] flags = "0" policy = "0" dh_pub_qx = "0123456789abcdef0123456789abcdef" dh_pub_qy = "0123456789abcdef0123456789abcdef" nonce = "0123456789abcdef" vcpu_count = "1" vcpu_length = "30" vcpu_mask = "00ab" One of the bit in policy field is "debugging", if this bit is set then hypervisor can use SEV commands to decrypt a guest memory otherwise hypervisor read will always get encrypted data. Also note that policy field is used by firmware when computing the measurement of a guest launch so any changes in policy by hypervisor will result in wrong measurement. > >> For more information see [1], section 7.1 >> >> [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf > > Please add comments documenting APIs. Spec links to figure out > implementation is one thing, but you really can't require people > to read specs just to figure out how to use an API. > Sure, i will work towards creating a simple file in doc/ directory that will list of commands, usage and their parameters and provide the link to exact section. >> The following KVM RFC patches defines and implements this command >> >> http://marc.info/?l=kvm&m=147190852423972&w=2 >> http://marc.info/?l=kvm&m=147191068524579&w=2 >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> include/sysemu/sev.h | 10 ++++++++++ >> sev.c | 23 +++++++++++++++++++++++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h >> index ab03c5d..5872c3e 100644 >> --- a/include/sysemu/sev.h >> +++ b/include/sysemu/sev.h >> @@ -55,4 +55,14 @@ int kvm_sev_guest_finish(void); >> */ >> int kvm_sev_guest_measurement(uint8_t *measurement); >> >> +/** >> + * kvm_sev_dbg_decrypt - decrypt the guest memory for debugging purposes >> + * @src - guest memory address >> + * @dest - host memory address where the decrypted data should be copied >> + * @length - length of memory region >> + * >> + * Returns: 0 on success and dest will contains the decrypted data >> + */ >> +int kvm_sev_dbg_decrypt(uint8_t *dest, const uint8_t *src, uint32_t len); >> + >> #endif >> diff --git a/sev.c b/sev.c >> index 055ed83..c7031d3 100644 >> --- a/sev.c >> +++ b/sev.c >> @@ -432,3 +432,26 @@ int kvm_sev_guest_measurement(uint8_t *out) >> >> return 0; >> } >> + >> +int kvm_sev_dbg_decrypt(uint8_t *dst, const uint8_t *src, uint32_t len) >> +{ >> + int ret; >> + struct kvm_sev_dbg_decrypt decrypt; >> + struct kvm_sev_issue_cmd input; >> + >> + decrypt.src_addr = (unsigned long)src; >> + decrypt.dst_addr = (unsigned long)dst; >> + decrypt.length = len; >> + >> + input.cmd = KVM_SEV_DBG_DECRYPT; >> + input.opaque = (unsigned long)&decrypt; >> + ret = kvm_vm_ioctl(kvm_state, KVM_SEV_ISSUE_CMD, &input); >> + if (ret) { >> + fprintf(stderr, "SEV: dbg_decrypt failed ret=%d(%#010x)\n", >> + ret, input.ret_code); >> + return 1; >> + } >> + >> + DPRINTF("SEV: DBG_DECRYPT dst %p src %p sz %d\n", dst, src, len); >> + return 0; >> +}
On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > I assumed that with debug on, memory is still encrypted but the > > > > hypervisor can break encryption, and as the cover letter states, the > > > > hypervisor is assumed benign. If true I don't see a need to > > > > give users more rope. > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > as possible for the attacker to do evil stuff to the guests. If the > > > attacker can just ask the secure processor "decrypt some memory for me", > > > then the encryption is effectively broken. > > > > So there's going to be a tradeoff here between use of SEV and use of > > certain other features. eg, it seems that if you're using SEV, then > > any concept of creating & analysing guest core dumps from the host > > is out. > > I don't see why - as long as we don't trigger dumps, there's no leak :) If the facility to trigger dumps is available, then the memory encryption feature of SEV is as useful as a chocolate teapot, as the would be attacker can simply trigger a dump bypassing any kind of SEV protection to get unencrypted memory. So if SEV is to provide any kind of useful security protection, there must be no way for a host admin to initiate core dumps of the guest, without first having some kind of explicit guest admin action to enable it. > > It seems that SEV on its own is insufficient - there is at least some > > interaction with storage. eg merely running a guest with SEV is not > > going to guarantee security if the guest OS is able to swap out to a > > non-encrypted disk. You could run LUKS inside the guest but that has > > a number of downsides. How to provide the decryption key for LUKS > > at startup without guest admin interaction. Then there is the issue > > that if you take snapshots of the guest disk in the host, this is > > weakening the security of LUKS, since you're keeping around copies > > of the same logical guest sector with different contents which > > allows for improve crytoanalysis. These are reasons for using LUKS > > on the host instead of in the guest, but then the decryption kjeys > > for LUKS are in the QEMU process in memory which is (IIUC) not going > > to be protected by SEV ? Unles there's a way for QEMU to do allocations > > which are SEV protected for its own purposes ? Regards, Daniel
On Wed, Sep 14, 2016 at 08:36:50AM -0500, Brijesh Singh wrote: > On 09/13/2016 09:28 PM, Michael S. Tsirkin wrote: > > On Tue, Sep 13, 2016 at 10:48:27AM -0400, Brijesh Singh wrote: > > > The SEV DEBUG_DECRYPT command is used for decrypting a guest memory > > > for the debugging purposes. Note that debugging is permitting only > > > when guest policy allows it. > > > > When wouldn't you want to allow it? > > I don't see value in a "break debugging" feature. > > > A guest owner needs to provide the launch parameters before we launch a SEV > guest, a typical input parameters looks like this. > > [sev-launch] > flags = "0" > policy = "0" > dh_pub_qx = "0123456789abcdef0123456789abcdef" > dh_pub_qy = "0123456789abcdef0123456789abcdef" > nonce = "0123456789abcdef" > vcpu_count = "1" > vcpu_length = "30" > vcpu_mask = "00ab" > > One of the bit in policy field is "debugging", if this bit is set then > hypervisor can use SEV commands to decrypt a guest memory That is my point. Arbitrary code execution in hypervisor means game over anyway, at least with the hardware we have today. If qemu gains a flag disabling a feature, it needs some documentation that explains: disabling this will break ABC but protect against XYZ. How do you expect people to use the feature otherwise? My suggestion is to merge the support for encrypting memory first, then make extras like disabling debugging on top. > otherwise > hypervisor read will always get encrypted data. Also note that policy field > is used by firmware when computing the measurement of a guest launch so any > changes in policy by hypervisor will result in wrong measurement. I can't say I understand how does guest measuring help prevent leaks in any way. Looks like a separate feature - why not split it out? > > > > > For more information see [1], section 7.1 > > > > > > [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf > > > > Please add comments documenting APIs. Spec links to figure out > > implementation is one thing, but you really can't require people > > to read specs just to figure out how to use an API. > > > Sure, i will work towards creating a simple file in doc/ directory that > will list of commands, usage and their parameters and provide the link to > exact section. > > > > The following KVM RFC patches defines and implements this command > > > > > > http://marc.info/?l=kvm&m=147190852423972&w=2 > > > http://marc.info/?l=kvm&m=147191068524579&w=2 > > > > > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > > --- > > > include/sysemu/sev.h | 10 ++++++++++ > > > sev.c | 23 +++++++++++++++++++++++ > > > 2 files changed, 33 insertions(+) > > > > > > diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h > > > index ab03c5d..5872c3e 100644 > > > --- a/include/sysemu/sev.h > > > +++ b/include/sysemu/sev.h > > > @@ -55,4 +55,14 @@ int kvm_sev_guest_finish(void); > > > */ > > > int kvm_sev_guest_measurement(uint8_t *measurement); > > > > > > +/** > > > + * kvm_sev_dbg_decrypt - decrypt the guest memory for debugging purposes > > > + * @src - guest memory address > > > + * @dest - host memory address where the decrypted data should be copied > > > + * @length - length of memory region > > > + * > > > + * Returns: 0 on success and dest will contains the decrypted data > > > + */ > > > +int kvm_sev_dbg_decrypt(uint8_t *dest, const uint8_t *src, uint32_t len); > > > + > > > #endif > > > diff --git a/sev.c b/sev.c > > > index 055ed83..c7031d3 100644 > > > --- a/sev.c > > > +++ b/sev.c > > > @@ -432,3 +432,26 @@ int kvm_sev_guest_measurement(uint8_t *out) > > > > > > return 0; > > > } > > > + > > > +int kvm_sev_dbg_decrypt(uint8_t *dst, const uint8_t *src, uint32_t len) > > > +{ > > > + int ret; > > > + struct kvm_sev_dbg_decrypt decrypt; > > > + struct kvm_sev_issue_cmd input; > > > + > > > + decrypt.src_addr = (unsigned long)src; > > > + decrypt.dst_addr = (unsigned long)dst; > > > + decrypt.length = len; > > > + > > > + input.cmd = KVM_SEV_DBG_DECRYPT; > > > + input.opaque = (unsigned long)&decrypt; > > > + ret = kvm_vm_ioctl(kvm_state, KVM_SEV_ISSUE_CMD, &input); > > > + if (ret) { > > > + fprintf(stderr, "SEV: dbg_decrypt failed ret=%d(%#010x)\n", > > > + ret, input.ret_code); > > > + return 1; > > > + } > > > + > > > + DPRINTF("SEV: DBG_DECRYPT dst %p src %p sz %d\n", dst, src, len); > > > + return 0; > > > +}
On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > give users more rope. > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > then the encryption is effectively broken. > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > certain other features. eg, it seems that if you're using SEV, then > > > any concept of creating & analysing guest core dumps from the host > > > is out. > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > If the facility to trigger dumps is available, then the memory > encryption feature of SEV is as useful as a chocolate teapot, > as the would be attacker can simply trigger a dump If attacker can trigger things, IOW execute code in hypervisor, then encrypting memory is not useful anyway. > bypassing > any kind of SEV protection to get unencrypted memory. So if > SEV is to provide any kind of useful security protection, there > must be no way for a host admin to initiate core dumps of the > guest, without first having some kind of explicit guest admin > action to enable it. As stated it protects against passive adversaries with read-only access to hypervisor memory. I don't see how dump ability breaks that. > > > It seems that SEV on its own is insufficient - there is at least some > > > interaction with storage. eg merely running a guest with SEV is not > > > going to guarantee security if the guest OS is able to swap out to a > > > non-encrypted disk. You could run LUKS inside the guest but that has > > > a number of downsides. How to provide the decryption key for LUKS > > > at startup without guest admin interaction. Then there is the issue > > > that if you take snapshots of the guest disk in the host, this is > > > weakening the security of LUKS, since you're keeping around copies > > > of the same logical guest sector with different contents which > > > allows for improve crytoanalysis. These are reasons for using LUKS > > > on the host instead of in the guest, but then the decryption kjeys > > > for LUKS are in the QEMU process in memory which is (IIUC) not going > > > to be protected by SEV ? Unles there's a way for QEMU to do allocations > > > which are SEV protected for its own purposes ? > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > give users more rope. > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > then the encryption is effectively broken. > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > certain other features. eg, it seems that if you're using SEV, then > > > > any concept of creating & analysing guest core dumps from the host > > > > is out. > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > If the facility to trigger dumps is available, then the memory > > encryption feature of SEV is as useful as a chocolate teapot, > > as the would be attacker can simply trigger a dump > > If attacker can trigger things, IOW execute code in hypervisor, > then encrypting memory is not useful anyway. I believe the whole point of SEV attestation and key management is to make "if attacker can executed code in hypervisor, encrypting memory is not useful" _not_ true, isn't it? Or are there known vulnerabilities that would allow a compromised hypervisor to decrypt memory even after successful encryption+attestation?
On 14/09/2016 16:08, Eduardo Habkost wrote: >> > If attacker can trigger things, IOW execute code in hypervisor, >> > then encrypting memory is not useful anyway. > I believe the whole point of SEV attestation and key management > is to make "if attacker can executed code in hypervisor, > encrypting memory is not useful" _not_ true, isn't it? > > Or are there known vulnerabilities that would allow a compromised > hypervisor to decrypt memory even after successful > encryption+attestation? There are countless side channels that you can use but you have to start somewhere, and anyway a side channel attack is way way more complex than just "trigger a debug dump and read it". Paolo
On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > give users more rope. > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > then the encryption is effectively broken. > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > certain other features. eg, it seems that if you're using SEV, then > > > > any concept of creating & analysing guest core dumps from the host > > > > is out. > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > If the facility to trigger dumps is available, then the memory > > encryption feature of SEV is as useful as a chocolate teapot, > > as the would be attacker can simply trigger a dump > > If attacker can trigger things, IOW execute code in hypervisor, > then encrypting memory is not useful anyway. The presentation at KVM forum claimed it *would* protect against this, and that things like core dump of unencrypted memory would not be permitted, so there's a disconnect between that preso and what you're saying. Regards, Daniel
On 14/09/2016 15:48, Michael S. Tsirkin wrote: >> One of the bit in policy field is "debugging", if this bit is set then >> hypervisor can use SEV commands to decrypt a guest memory > > That is my point. Arbitrary code execution in hypervisor means game over > anyway, at least with the hardware we have today. Game is over if you assume the attacker has infinite power. In practice the attacker may be limited by other security features (SELinux, seccomp, external firewalls, whatever), by the money and time they can spend on the attack. So anything that makes things harder for the attacker is a security improvement. > My suggestion is to merge the support for encrypting memory first, > then make extras like disabling debugging on top. Sorry but I concur with others that this makes no sense at all. If anything, it's *enabling* debugging that can be done on top. That said... > I can't say I understand how does guest measuring help prevent > leaks in any way. Looks like a separate feature - why not split it > out? ... the patch series seems to be pretty small and self contained. I don't see any point in splitting it further. Paolo
On Wed, Sep 14, 2016 at 04:14:04PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 16:08, Eduardo Habkost wrote: > >> > If attacker can trigger things, IOW execute code in hypervisor, > >> > then encrypting memory is not useful anyway. > > I believe the whole point of SEV attestation and key management > > is to make "if attacker can executed code in hypervisor, > > encrypting memory is not useful" _not_ true, isn't it? > > > > Or are there known vulnerabilities that would allow a compromised > > hypervisor to decrypt memory even after successful > > encryption+attestation? > > There are countless side channels that you can use but you have to start > somewhere, Absolutely, so let's start with a feature that is orthogonal, has a defined threat model and does not conflict with valid uses please. I was very happy to see an actual threat documented (passive adversary with read only capabilities) as opposed to a vague makes some attacks harder. Why don't we merge a patchset with that, and then add stuff on top, documenting the benefits at each step? > and anyway a side channel attack is way way more complex More complex, sure, but in the age of libraries of exploits, I'm not convinced it is measureably *harder* even if you add a third "way" in this sentence. 0 multiplied by 1000 is still 0. > than > just "trigger a debug dump and read it". > > Paolo Really, my point isn't that ability to disable debugging is useless. My point is that the feature is not really related to memory encryption except by the vague "both are security things" notion. If you consider adversary that has access to the monitor and nothing else, then apparently disabling dumps and debugging might be useful. So don't tie it all in to SEV please.
On Wed, Sep 14, 2016 at 03:15:07PM +0100, Daniel P. Berrange wrote: > On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > > give users more rope. > > > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > > then the encryption is effectively broken. > > > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > > certain other features. eg, it seems that if you're using SEV, then > > > > > any concept of creating & analysing guest core dumps from the host > > > > > is out. > > > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > > > If the facility to trigger dumps is available, then the memory > > > encryption feature of SEV is as useful as a chocolate teapot, > > > as the would be attacker can simply trigger a dump > > > > If attacker can trigger things, IOW execute code in hypervisor, > > then encrypting memory is not useful anyway. > > The presentation at KVM forum claimed it *would* protect against > this, and that things like core dump of unencrypted memory would > not be permitted, so there's a disconnect between that preso and > what you're saying. > > Regards, > Daniel You mean presentation claimed protection against leaks to a malicious active attacker within a hypervisor? I guess the presentation covers more than this patchset does then. And the disconnect would be with what the patchset cover letter says, not just with what I say. Clearly encrypting memory is not enough to protect against a malicious hypervisor. E.g. just running info cpus is enough to leak information from guest. > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Sep 14, 2016 at 04:19:38PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 15:48, Michael S. Tsirkin wrote: > >> One of the bit in policy field is "debugging", if this bit is set then > >> hypervisor can use SEV commands to decrypt a guest memory > > > > That is my point. Arbitrary code execution in hypervisor means game over > > anyway, at least with the hardware we have today. > > Game is over if you assume the attacker has infinite power. In practice > the attacker may be limited by other security features (SELinux, > seccomp, external firewalls, whatever), by the money and time they can > spend on the attack. So anything that makes things harder for the > attacker is a security improvement. Why don't we add assert(i == X) after each i = X in code? This will make things harder for attackers that can skip a single instruction only. I think that the answer is that's because we do not believe there are such attackers. So defining a threat model is important. The one that this patchset cover letter defined does not include active attackers. > > My suggestion is to merge the support for encrypting memory first, > > then make extras like disabling debugging on top. > > Sorry but I concur with others that this makes no sense at all. If > anything, it's *enabling* debugging that can be done on top. That said... > > > I can't say I understand how does guest measuring help prevent > > leaks in any way. Looks like a separate feature - why not split it > > out? > > ... the patch series seems to be pretty small and self contained. I > don't see any point in splitting it further. > > Paolo One point is so we can discuss features and generalize them. If you believe there are attackers that have access to the monitor and nothing else, then a feature to disable debugging is a generally useful one. But once we merge sev patchset then of course sev people disappear and it will be up to others to make it work on non-amd CPUs. Another is to help merge other parts faster. E.g. looking at what Daniel writes, the feature might have been over-sold so people will disable debugging thinking this will prevent all active attacks. Thus we now need to add good documentation so people know what they can actually expect to get from QEMU in return for disabling debugging. Why not merge the simple "encrypt memory part" while this documentation work is going on?
On Wed, Sep 14, 2016 at 05:48:17PM +0300, Michael S. Tsirkin wrote: > On Wed, Sep 14, 2016 at 03:15:07PM +0100, Daniel P. Berrange wrote: > > On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > > > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > > > give users more rope. > > > > > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > > > then the encryption is effectively broken. > > > > > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > > > certain other features. eg, it seems that if you're using SEV, then > > > > > > any concept of creating & analysing guest core dumps from the host > > > > > > is out. > > > > > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > > > > > If the facility to trigger dumps is available, then the memory > > > > encryption feature of SEV is as useful as a chocolate teapot, > > > > as the would be attacker can simply trigger a dump > > > > > > If attacker can trigger things, IOW execute code in hypervisor, > > > then encrypting memory is not useful anyway. > > > > The presentation at KVM forum claimed it *would* protect against > > this, and that things like core dump of unencrypted memory would > > not be permitted, so there's a disconnect between that preso and > > what you're saying. > > > > Regards, > > Daniel > > You mean presentation claimed protection against leaks to a malicious > active attacker within a hypervisor? I guess the presentation covers > more than this patchset does then. And the disconnect would be with > what the patchset cover letter says, not just with what I say. Clearly > encrypting memory is not enough to protect against a malicious > hypervisor. E.g. just running info cpus is enough to leak information > from guest. It was explicit about the fact that the host admin would not have any way to get access to the full contents of guest memory, without the guest admin granting it. Only those non-encrypted pages used for I/O transfer between host & guest would be accessible. Regards, Daniel
On Wed, Sep 14, 2016 at 11:08:45AM -0300, Eduardo Habkost wrote: > On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > > give users more rope. > > > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > > then the encryption is effectively broken. > > > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > > certain other features. eg, it seems that if you're using SEV, then > > > > > any concept of creating & analysing guest core dumps from the host > > > > > is out. > > > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > > > If the facility to trigger dumps is available, then the memory > > > encryption feature of SEV is as useful as a chocolate teapot, > > > as the would be attacker can simply trigger a dump > > > > If attacker can trigger things, IOW execute code in hypervisor, > > then encrypting memory is not useful anyway. > > I believe the whole point of SEV attestation and key management > is to make "if attacker can executed code in hypervisor, > encrypting memory is not useful" _not_ true, isn't it? That would be an aggressive claim. Not the one the cover letter is making. > Or are there known vulnerabilities that would allow a compromised > hypervisor to decrypt memory even after successful > encryption+attestation? > -- > Eduardo
On Wed, Sep 14, 2016 at 04:06:33PM +0100, Daniel P. Berrange wrote: > On Wed, Sep 14, 2016 at 05:48:17PM +0300, Michael S. Tsirkin wrote: > > On Wed, Sep 14, 2016 at 03:15:07PM +0100, Daniel P. Berrange wrote: > > > On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > > > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > > > > give users more rope. > > > > > > > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > > > > then the encryption is effectively broken. > > > > > > > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > > > > certain other features. eg, it seems that if you're using SEV, then > > > > > > > any concept of creating & analysing guest core dumps from the host > > > > > > > is out. > > > > > > > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > > > > > > > If the facility to trigger dumps is available, then the memory > > > > > encryption feature of SEV is as useful as a chocolate teapot, > > > > > as the would be attacker can simply trigger a dump > > > > > > > > If attacker can trigger things, IOW execute code in hypervisor, > > > > then encrypting memory is not useful anyway. > > > > > > The presentation at KVM forum claimed it *would* protect against > > > this, and that things like core dump of unencrypted memory would > > > not be permitted, so there's a disconnect between that preso and > > > what you're saying. > > > > > > Regards, > > > Daniel > > > > You mean presentation claimed protection against leaks to a malicious > > active attacker within a hypervisor? I guess the presentation covers > > more than this patchset does then. And the disconnect would be with > > what the patchset cover letter says, not just with what I say. Clearly > > encrypting memory is not enough to protect against a malicious > > hypervisor. E.g. just running info cpus is enough to leak information > > from guest. > > It was explicit about the fact that the host admin would not have any > way to get access to the full contents of guest memory, without the > guest admin granting it. Only those non-encrypted pages used for I/O > transfer between host & guest would be accessible. > > Regards, > Daniel If you like, that's the vision. I'd rather discuss the patchset in question though. It encrypts all memory but this does not protect against all attackers, only passive ones. If you disable debugging, it seems to additionally reduce the amount of information that can be leaked to an active attacker in the hypervisor at one go. Paolo seems to think it's useful, but it's a far cry from a deal breaker, and your email just makes me worry that it has been oversold to the point where everyone will start disabling debugging everywhere in production and claim that otherwise it's a security problem. IMO a much better in-tree documentation is needed so people know what they are getting in return. Attestation seems mostly unrelated. The whitepaper says With this attestation, a guest owner can ensure that the hypervisor did not interfere with the initialization of SEV before transmitting confidential information to the guest. which seems to imply an active attacker that is able to interfere with the hypervisor during guest initialization but not afterwards. So I have no idea why that's useful at the moment - I suspect it's part of the future vision when there are protections against all active attackers in place, but for now it seems to extend the firmware/software interface unnecessarily. > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 14/09/2016 17:02, Michael S. Tsirkin wrote: > If you believe there are attackers that have access to the > monitor and nothing else, then a feature to disable debugging > is a generally useful one. But once we merge sev patchset then of course > sev people disappear and it will be up to others to make it > work on non-amd CPUs. > > Another is to help merge other parts faster. E.g. looking at what > Daniel writes, the feature might have been over-sold so people will > disable debugging thinking this will prevent all active attacks. Thus we > now need to add good documentation so people know what they can actually > expect to get from QEMU in return for disabling debugging. Why not merge > the simple "encrypt memory part" while this documentation work is going > on? Encrypting memory makes no sense if anyone can ask to decrypt it. And I'm not even sure how force-enabling debug r/w, which is literally a single bit set in the feature register, would make the patchset simpler. If anything, as I said already, it would make the patchset simpler to force-*disable* it, since you don't need to introduce debug hooks that go through the secure processor. Paolo
On Wed, Sep 14, 2016 at 06:46:20PM +0300, Michael S. Tsirkin wrote: > On Wed, Sep 14, 2016 at 04:06:33PM +0100, Daniel P. Berrange wrote: > > On Wed, Sep 14, 2016 at 05:48:17PM +0300, Michael S. Tsirkin wrote: > > > On Wed, Sep 14, 2016 at 03:15:07PM +0100, Daniel P. Berrange wrote: > > > > On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > > > > > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > > > > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > > > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > > > > > give users more rope. > > > > > > > > > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > > > > > then the encryption is effectively broken. > > > > > > > > > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > > > > > certain other features. eg, it seems that if you're using SEV, then > > > > > > > > any concept of creating & analysing guest core dumps from the host > > > > > > > > is out. > > > > > > > > > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > > > > > > > > > If the facility to trigger dumps is available, then the memory > > > > > > encryption feature of SEV is as useful as a chocolate teapot, > > > > > > as the would be attacker can simply trigger a dump > > > > > > > > > > If attacker can trigger things, IOW execute code in hypervisor, > > > > > then encrypting memory is not useful anyway. > > > > > > > > The presentation at KVM forum claimed it *would* protect against > > > > this, and that things like core dump of unencrypted memory would > > > > not be permitted, so there's a disconnect between that preso and > > > > what you're saying. > > > > > > > > Regards, > > > > Daniel > > > > > > You mean presentation claimed protection against leaks to a malicious > > > active attacker within a hypervisor? I guess the presentation covers > > > more than this patchset does then. And the disconnect would be with > > > what the patchset cover letter says, not just with what I say. Clearly > > > encrypting memory is not enough to protect against a malicious > > > hypervisor. E.g. just running info cpus is enough to leak information > > > from guest. > > > > It was explicit about the fact that the host admin would not have any > > way to get access to the full contents of guest memory, without the > > guest admin granting it. Only those non-encrypted pages used for I/O > > transfer between host & guest would be accessible. > > > > Regards, > > Daniel > > If you like, that's the vision. I'd rather discuss the patchset in > question though. It encrypts all memory but this does not protect against > all attackers, only passive ones. If you disable debugging, > it seems to additionally reduce the amount of information that can be > leaked to an active attacker in the hypervisor at one go. > > Paolo seems to think it's useful, but it's a far cry from a deal > breaker, and your email just makes me worry that it has been oversold to > the point where everyone will start disabling debugging everywhere in > production and claim that otherwise it's a security problem. IMO a much > better in-tree documentation is needed so people know what they are > getting in return. > > Attestation seems mostly unrelated. The whitepaper says > With this attestation, a guest owner can ensure that the hypervisor did > not interfere with the initialization of SEV before transmitting > confidential information to the guest. > which seems to imply an active attacker that is able to interfere > with the hypervisor during guest initialization but not afterwards. I believe this assumes a compromised hypervisor both before and after guest launch, but this assumes the hypervisor: 1) Won't be able to change guest memory before attestation without being detected. 2) Won't be able to attack the guest after memory is encrypted. > So I have no idea why that's useful at the moment - I suspect > it's part of the future vision when there are protections > against all active attackers in place, but for now it seems to extend the > firmware/software interface unnecessarily. "Protection against all active attackers" is a very broad requirement. Effective protection against a given subset of attacks would be reasonable enough to me.
On Wed, Sep 14, 2016 at 06:53:22PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 17:02, Michael S. Tsirkin wrote: > > If you believe there are attackers that have access to the > > monitor and nothing else, then a feature to disable debugging > > is a generally useful one. But once we merge sev patchset then of course > > sev people disappear and it will be up to others to make it > > work on non-amd CPUs. > > > > Another is to help merge other parts faster. E.g. looking at what > > Daniel writes, the feature might have been over-sold so people will > > disable debugging thinking this will prevent all active attacks. Thus we > > now need to add good documentation so people know what they can actually > > expect to get from QEMU in return for disabling debugging. Why not merge > > the simple "encrypt memory part" while this documentation work is going > > on? > > Encrypting memory makes no sense if anyone can ask to decrypt it. It's not useless since the attack model here is a passive adversary that can not ask anything. > And > I'm not even sure how force-enabling debug r/w, which is literally a > single bit set in the feature register, would make the patchset simpler. It will make the *interface* simpler. > If anything, as I said already, it would make the patchset simpler to > force-*disable* it, since you don't need to introduce debug hooks that > go through the secure processor. > > Paolo My suggestion is to add a processor independent hook that disables debugging. Arguably this improves security in case attacker only has access to the monitor.
On 14/09/2016 20:15, Michael S. Tsirkin wrote: > On Wed, Sep 14, 2016 at 06:53:22PM +0200, Paolo Bonzini wrote: >> >> >> On 14/09/2016 17:02, Michael S. Tsirkin wrote: >>> If you believe there are attackers that have access to the >>> monitor and nothing else, then a feature to disable debugging >>> is a generally useful one. But once we merge sev patchset then of course >>> sev people disappear and it will be up to others to make it >>> work on non-amd CPUs. >>> >>> Another is to help merge other parts faster. E.g. looking at what >>> Daniel writes, the feature might have been over-sold so people will >>> disable debugging thinking this will prevent all active attacks. Thus we >>> now need to add good documentation so people know what they can actually >>> expect to get from QEMU in return for disabling debugging. Why not merge >>> the simple "encrypt memory part" while this documentation work is going >>> on? >> >> Encrypting memory makes no sense if anyone can ask to decrypt it. > > It's not useless since the attack model here is a passive adversary > that can not ask anything. Does _that attack model_ make sense then? Also, I don't think this is the attack model; limited protection against a compromised hypervisor is included. If the adversary is passive and cannot ask anything is it even an adversary? Why do you need encryption at all if you can't even ptrace QEMU? >> And >> I'm not even sure how force-enabling debug r/w, which is literally a >> single bit set in the feature register, would make the patchset simpler. > > It will make the *interface* simpler. If we made debug r/w force-disabled, the interface would be just as simple, and the outcome more secure and more sensible. >> If anything, as I said already, it would make the patchset simpler to >> force-*disable* it, since you don't need to introduce debug hooks that >> go through the secure processor. > > My suggestion is to add a processor independent hook that disables > debugging. Arguably this improves security in case attacker only has > access to the monitor. The default is the wrong direction though for encrypted guests... Paolo
On Wed, Sep 14, 2016 at 08:45:25PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 20:15, Michael S. Tsirkin wrote: > > On Wed, Sep 14, 2016 at 06:53:22PM +0200, Paolo Bonzini wrote: > >> > >> > >> On 14/09/2016 17:02, Michael S. Tsirkin wrote: > >>> If you believe there are attackers that have access to the > >>> monitor and nothing else, then a feature to disable debugging > >>> is a generally useful one. But once we merge sev patchset then of course > >>> sev people disappear and it will be up to others to make it > >>> work on non-amd CPUs. > >>> > >>> Another is to help merge other parts faster. E.g. looking at what > >>> Daniel writes, the feature might have been over-sold so people will > >>> disable debugging thinking this will prevent all active attacks. Thus we > >>> now need to add good documentation so people know what they can actually > >>> expect to get from QEMU in return for disabling debugging. Why not merge > >>> the simple "encrypt memory part" while this documentation work is going > >>> on? > >> > >> Encrypting memory makes no sense if anyone can ask to decrypt it. > > > > It's not useless since the attack model here is a passive adversary > > that can not ask anything. > > Does _that attack model_ make sense then? It seems to make sense superficially. > Also, I don't think this is > the attack model; limited protection against a compromised hypervisor is > included. Well limited protection is of a limited use :) Seriously, the point of mitigation should be blocking classes of vulenrabilities not making things more complex. > If the adversary is passive and cannot ask anything is it even an > adversary? Why do you need encryption at all if you can't even ptrace QEMU? The cover letter mentioned a read everything adversary. How do you read everything? Well, you probably don't but there could be attacks that cause kernel to leak contents of random memory to an attacker. > >> And > >> I'm not even sure how force-enabling debug r/w, which is literally a > >> single bit set in the feature register, would make the patchset simpler. > > > > It will make the *interface* simpler. > > If we made debug r/w force-disabled, the interface would be just as > simple, and the outcome more secure and more sensible. If you don't think debugging is useful (maybe it isn't) do it for everyone then :) > >> If anything, as I said already, it would make the patchset simpler to > >> force-*disable* it, since you don't need to introduce debug hooks that > >> go through the secure processor. > > > > My suggestion is to add a processor independent hook that disables > > debugging. Arguably this improves security in case attacker only has > > access to the monitor. > > The default is the wrong direction though for encrypted guests... > > Paolo I think this is just tying unrelated features together. Hardware vendors always do this - they want to sell their hardware that solves all the problems. On the software side, we should try to push for enabling features independently, this way more hardware can benefit. People that do not need debugging can disable it and maybe some exploit will be prevented. Not at all different for encryption.
On 14/09/2016 21:24, Michael S. Tsirkin wrote: > Well limited protection is of a limited use :) Seriously, the point of > mitigation should be blocking classes of vulenrabilities not making > things more complex. No, not at all. The point of _mitigation_ is to _mitigate_ the danger from classes of vulnerabilities, i.e. make the attack harder though perhaps not ultimately impossible. >> If the adversary is passive and cannot ask anything is it even an >> adversary? Why do you need encryption at all if you can't even ptrace QEMU? > > The cover letter mentioned a read everything adversary. > How do you read everything? Well, you probably don't but > there could be attacks that cause kernel to leak > contents of random memory to an attacker. Ok, it doesn't seem too useful. > On the software side, we should try to > push for enabling features independently, this way more > hardware can benefit. We can have an "unencrypted" sev-policy that only has limited functionality such as disabling debug. So you could disable debug with -object sev-policy-unencrypted,debug=false,id=mypolicy \ -machine ...,sev-policy=mypolicy Paolo
On Wed, Sep 14, 2016 at 09:58:25PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 21:24, Michael S. Tsirkin wrote: > > Well limited protection is of a limited use :) Seriously, the point of > > mitigation should be blocking classes of vulenrabilities not making > > things more complex. > > No, not at all. The point of _mitigation_ is to _mitigate_ the danger > from classes of vulnerabilities, i.e. make the attack harder though > perhaps not ultimately impossible. Right. And features generally reduce security. Does not mean we don't need to add any features. The tradeoffs need to be weighted and documented, this is missing here. Specifically with debug, if you have debug then clearly you can dump guest memory. This is what this feature is about. If we want a hypervisor that can not dump guest memory, let's add a flag like that. Does everyone have to disable debugging by default? I don't see why. Does everyone using encryption have to do this? I don't see why either. > >> If the adversary is passive and cannot ask anything is it even an > >> adversary? Why do you need encryption at all if you can't even ptrace QEMU? > > > > The cover letter mentioned a read everything adversary. > > How do you read everything? Well, you probably don't but > > there could be attacks that cause kernel to leak > > contents of random memory to an attacker. > > Ok, it doesn't seem too useful. > > > On the software side, we should try to > > push for enabling features independently, this way more > > hardware can benefit. > > We can have an "unencrypted" sev-policy that only has limited > functionality such as disabling debug. So you could disable debug with > > -object sev-policy-unencrypted,debug=false,id=mypolicy \ > -machine ...,sev-policy=mypolicy > > Paolo I wouldn't say sev on the command line. SEV seems to be a group of AMD technologies implemening memory encryption, measurement etc. Let's have flags for individual components: -machine ...,debug=false,memory-encryption=on,... E.g. I can imagine tcg implementing encrypted at rest memory. If you are on AMD and memory=encrypted then you would enable SEV. If debug=false then disable debug. As I mentioned, if monitor is a socket this might be genuinely increasing guest security. I'm fine with e.g. memory-encryption=on being an AMD-only feature for now.
On 14/09/2016 22:36, Michael S. Tsirkin wrote: > Specifically with debug, if you have debug then clearly you > can dump guest memory. This is what this feature is about. > If we want a hypervisor that can not dump guest memory, let's > add a flag like that. Does everyone have to disable debugging > by default? I don't see why. Does everyone using encryption > have to do this? I don't see why either. If you can explain what's the point in doing encryption that can be defeated with a single ioctl, perhaps I'll agree with you. It's okay that we leave out features. But every feature left out is an anti-feature baked in. Force-enable debug? You've provided a loophole for everyone. Force-disable debug? Well, of course you've blocked debug for everyone. I agree that they are distinct features on the command line, but I think you're underestimating the importance of choosing a sane default, that's it. >> -object sev-policy-unencrypted,debug=false,id=mypolicy \ >> -machine ...,sev-policy=mypolicy > > I wouldn't say sev on the command line. SEV seems to be > a group of AMD technologies implemening memory encryption, > measurement etc. > > Let's have flags for individual components: > > -machine ...,debug=false,memory-encryption=on,... I think it makes sense to have a separate -object for the policy. Let's just make it security-policy instead of sev-policy. Brijesh, is that okay? Paolo
On 09/14/2016 03:44 PM, Paolo Bonzini wrote: > > > On 14/09/2016 22:36, Michael S. Tsirkin wrote: >> Specifically with debug, if you have debug then clearly you >> can dump guest memory. This is what this feature is about. >> If we want a hypervisor that can not dump guest memory, let's >> add a flag like that. Does everyone have to disable debugging >> by default? I don't see why. Does everyone using encryption >> have to do this? I don't see why either. > > If you can explain what's the point in doing encryption that can be > defeated with a single ioctl, perhaps I'll agree with you. It's okay > that we leave out features. But every feature left out is an > anti-feature baked in. Force-enable debug? You've provided a loophole > for everyone. Force-disable debug? Well, of course you've blocked > debug for everyone. > > I agree that they are distinct features on the command line, but I think > you're underestimating the importance of choosing a sane default, that's it. > >>> -object sev-policy-unencrypted,debug=false,id=mypolicy \ >>> -machine ...,sev-policy=mypolicy >> >> I wouldn't say sev on the command line. SEV seems to be >> a group of AMD technologies implemening memory encryption, >> measurement etc. >> >> Let's have flags for individual components: >> >> -machine ...,debug=false,memory-encryption=on,... > > I think it makes sense to have a separate -object for the policy. Let's > just make it security-policy instead of sev-policy. Brijesh, is that okay? > Yes, fine with me. -Brijesh
On Wed, Sep 14, 2016 at 10:44:58PM +0200, Paolo Bonzini wrote: > > > On 14/09/2016 22:36, Michael S. Tsirkin wrote: > > Specifically with debug, if you have debug then clearly you > > can dump guest memory. This is what this feature is about. > > If we want a hypervisor that can not dump guest memory, let's > > add a flag like that. Does everyone have to disable debugging > > by default? I don't see why. Does everyone using encryption > > have to do this? I don't see why either. > > If you can explain what's the point in doing encryption that can be > defeated with a single ioctl, perhaps I'll agree with you. Discussed offline, I hope I clarified things. Hypervisor (host kernel) can decrypt but it is already possible for it to cause guest info leaks. But no one else on the host can. > It's okay > that we leave out features. But every feature left out is an > anti-feature baked in. Force-enable debug? You've provided a loophole > for everyone. It's already baked in by default. Let's switch it to off by default for everyone if we are worried about using monitor to leak guest secrets? Btw with a TCP socket monitor, this seems like a legitimate worry. We can do it when the new security policy object is created. > Force-disable debug? Well, of course you've blocked > debug for everyone. > > I agree that they are distinct features on the command line, but I think > you're underestimating the importance of choosing a sane default, that's it. We can safely leave that for management, but I won't object to switching the default too, let's just do it for everyone, consistently. > >> -object sev-policy-unencrypted,debug=false,id=mypolicy \ > >> -machine ...,sev-policy=mypolicy > > > > I wouldn't say sev on the command line. SEV seems to be > > a group of AMD technologies implemening memory encryption, > > measurement etc. > > > > Let's have flags for individual components: > > > > -machine ...,debug=false,memory-encryption=on,... > > I think it makes sense to have a separate -object for the policy. Let's > just make it security-policy instead of sev-policy. Brijesh, is that okay? > > Paolo OK. And some parts like blocking debug are easy enough to implement for everyone.
On Wed, Sep 14, 2016 at 02:35:41PM -0300, Eduardo Habkost wrote: > On Wed, Sep 14, 2016 at 06:46:20PM +0300, Michael S. Tsirkin wrote: > > On Wed, Sep 14, 2016 at 04:06:33PM +0100, Daniel P. Berrange wrote: > > > On Wed, Sep 14, 2016 at 05:48:17PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, Sep 14, 2016 at 03:15:07PM +0100, Daniel P. Berrange wrote: > > > > > On Wed, Sep 14, 2016 at 04:50:51PM +0300, Michael S. Tsirkin wrote: > > > > > > On Wed, Sep 14, 2016 at 02:37:49PM +0100, Daniel P. Berrange wrote: > > > > > > > On Wed, Sep 14, 2016 at 04:32:44PM +0300, Michael S. Tsirkin wrote: > > > > > > > > On Wed, Sep 14, 2016 at 02:23:14PM +0100, Daniel P. Berrange wrote: > > > > > > > > > On Wed, Sep 14, 2016 at 03:07:58PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 14/09/2016 15:05, Michael S. Tsirkin wrote: > > > > > > > > > > > I assumed that with debug on, memory is still encrypted but the > > > > > > > > > > > hypervisor can break encryption, and as the cover letter states, the > > > > > > > > > > > hypervisor is assumed benign. If true I don't see a need to > > > > > > > > > > > give users more rope. > > > > > > > > > > > > > > > > > > > > The hypervisor is assumed benign but vulnerable. > > > > > > > > > > > > > > > > > > > > So, if somebody breaks the hypervisor, you would like to make it as hard > > > > > > > > > > as possible for the attacker to do evil stuff to the guests. If the > > > > > > > > > > attacker can just ask the secure processor "decrypt some memory for me", > > > > > > > > > > then the encryption is effectively broken. > > > > > > > > > > > > > > > > > > So there's going to be a tradeoff here between use of SEV and use of > > > > > > > > > certain other features. eg, it seems that if you're using SEV, then > > > > > > > > > any concept of creating & analysing guest core dumps from the host > > > > > > > > > is out. > > > > > > > > > > > > > > > > I don't see why - as long as we don't trigger dumps, there's no leak :) > > > > > > > > > > > > > > If the facility to trigger dumps is available, then the memory > > > > > > > encryption feature of SEV is as useful as a chocolate teapot, > > > > > > > as the would be attacker can simply trigger a dump > > > > > > > > > > > > If attacker can trigger things, IOW execute code in hypervisor, > > > > > > then encrypting memory is not useful anyway. > > > > > > > > > > The presentation at KVM forum claimed it *would* protect against > > > > > this, and that things like core dump of unencrypted memory would > > > > > not be permitted, so there's a disconnect between that preso and > > > > > what you're saying. > > > > > > > > > > Regards, > > > > > Daniel > > > > > > > > You mean presentation claimed protection against leaks to a malicious > > > > active attacker within a hypervisor? I guess the presentation covers > > > > more than this patchset does then. And the disconnect would be with > > > > what the patchset cover letter says, not just with what I say. Clearly > > > > encrypting memory is not enough to protect against a malicious > > > > hypervisor. E.g. just running info cpus is enough to leak information > > > > from guest. > > > > > > It was explicit about the fact that the host admin would not have any > > > way to get access to the full contents of guest memory, without the > > > guest admin granting it. Only those non-encrypted pages used for I/O > > > transfer between host & guest would be accessible. > > > > > > Regards, > > > Daniel > > > > If you like, that's the vision. I'd rather discuss the patchset in > > question though. It encrypts all memory but this does not protect against > > all attackers, only passive ones. If you disable debugging, > > it seems to additionally reduce the amount of information that can be > > leaked to an active attacker in the hypervisor at one go. > > > > Paolo seems to think it's useful, but it's a far cry from a deal > > breaker, and your email just makes me worry that it has been oversold to > > the point where everyone will start disabling debugging everywhere in > > production and claim that otherwise it's a security problem. IMO a much > > better in-tree documentation is needed so people know what they are > > getting in return. > > > > Attestation seems mostly unrelated. The whitepaper says > > With this attestation, a guest owner can ensure that the hypervisor did > > not interfere with the initialization of SEV before transmitting > > confidential information to the guest. > > which seems to imply an active attacker that is able to interfere > > with the hypervisor during guest initialization but not afterwards. > > I believe this assumes a compromised hypervisor both before and > after guest launch, but this assumes the hypervisor: > 1) Won't be able to change guest memory before attestation > without being detected. > 2) Won't be able to attack the guest after memory is encrypted. Why would you need to measure things then? If you assume this, at what point *can* attacker change memory? > > So I have no idea why that's useful at the moment - I suspect > > it's part of the future vision when there are protections > > against all active attackers in place, but for now it seems to extend the > > firmware/software interface unnecessarily. > > "Protection against all active attackers" is a very broad > requirement. Effective protection against a given subset of > attacks would be reasonable enough to me. Well selecting a random point in time and saying "I protect against attacks at this point only" would be a very weak protection, akin to just adding an assert statement at a random place in code - even though yes, if you hit that assert you are protected. This is not to say this is what this patchset does, merely that it should include a bit more information about the motivation for the measurement part than "this is what we can easily implement". > -- > Eduardo
On Thu, Sep 15, 2016 at 01:05:12AM +0300, Michael S. Tsirkin wrote: [...] > > > Attestation seems mostly unrelated. The whitepaper says > > > With this attestation, a guest owner can ensure that the hypervisor did > > > not interfere with the initialization of SEV before transmitting > > > confidential information to the guest. > > > which seems to imply an active attacker that is able to interfere > > > with the hypervisor during guest initialization but not afterwards. > > > > I believe this assumes a compromised hypervisor both before and > > after guest launch, but this assumes the hypervisor: > > 1) Won't be able to change guest memory before attestation > > without being detected. > > 2) Won't be able to attack the guest after memory is encrypted. > > Why would you need to measure things then? If you assume this, at what > point *can* attacker change memory? I am assuming an attacker that can change memory at any moment. If memory is changed before encryption, measurement/attestation would detect it. And I assume that memory changes after encryption won't cause much damage except crashing the guest. > > > > So I have no idea why that's useful at the moment - I suspect > > > it's part of the future vision when there are protections > > > against all active attackers in place, but for now it seems to extend the > > > firmware/software interface unnecessarily. > > > > "Protection against all active attackers" is a very broad > > requirement. Effective protection against a given subset of > > attacks would be reasonable enough to me. > > Well selecting a random point in time and saying "I protect against > attacks at this point only" would be a very weak protection, akin to > just adding an assert statement at a random place in code - even though > yes, if you hit that assert you are protected. > > This is not to say this is what this patchset does, merely > that it should include a bit more information about the > motivation for the measurement part than > "this is what we can easily implement". Agreed that we need more information about the attacks they have in mind.
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h index ab03c5d..5872c3e 100644 --- a/include/sysemu/sev.h +++ b/include/sysemu/sev.h @@ -55,4 +55,14 @@ int kvm_sev_guest_finish(void); */ int kvm_sev_guest_measurement(uint8_t *measurement); +/** + * kvm_sev_dbg_decrypt - decrypt the guest memory for debugging purposes + * @src - guest memory address + * @dest - host memory address where the decrypted data should be copied + * @length - length of memory region + * + * Returns: 0 on success and dest will contains the decrypted data + */ +int kvm_sev_dbg_decrypt(uint8_t *dest, const uint8_t *src, uint32_t len); + #endif diff --git a/sev.c b/sev.c index 055ed83..c7031d3 100644 --- a/sev.c +++ b/sev.c @@ -432,3 +432,26 @@ int kvm_sev_guest_measurement(uint8_t *out) return 0; } + +int kvm_sev_dbg_decrypt(uint8_t *dst, const uint8_t *src, uint32_t len) +{ + int ret; + struct kvm_sev_dbg_decrypt decrypt; + struct kvm_sev_issue_cmd input; + + decrypt.src_addr = (unsigned long)src; + decrypt.dst_addr = (unsigned long)dst; + decrypt.length = len; + + input.cmd = KVM_SEV_DBG_DECRYPT; + input.opaque = (unsigned long)&decrypt; + ret = kvm_vm_ioctl(kvm_state, KVM_SEV_ISSUE_CMD, &input); + if (ret) { + fprintf(stderr, "SEV: dbg_decrypt failed ret=%d(%#010x)\n", + ret, input.ret_code); + return 1; + } + + DPRINTF("SEV: DBG_DECRYPT dst %p src %p sz %d\n", dst, src, len); + return 0; +}
The SEV DEBUG_DECRYPT command is used for decrypting a guest memory for the debugging purposes. Note that debugging is permitting only when guest policy allows it. For more information see [1], section 7.1 [1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Spec.pdf The following KVM RFC patches defines and implements this command http://marc.info/?l=kvm&m=147190852423972&w=2 http://marc.info/?l=kvm&m=147191068524579&w=2 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- include/sysemu/sev.h | 10 ++++++++++ sev.c | 23 +++++++++++++++++++++++ 2 files changed, 33 insertions(+)