Re: [Qemu-devel] [RFC PATCH v1 10/22] sev: add SEV debug decrypt command

[Michael S. Tsirkin]

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 <address@hidden>
> >> ---
> >>  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;
> >> +}
> > 
> >