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

["Michael S. Tsirkin" <mst@redhat.com>]

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;
> > > +}