Re: [PATCH v11 21/28] sev/i386: add debug encrypt and decrypt commands

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	kvm@vger.kernel.org, "Michael S. Tsirkin" <mst@redhat.com>,
	Stefan Hajnoczi <stefanha@gmail.com>,
	qemu-devel@nongnu.org, Alexander Graf <agraf@suse.de>,
	"Edgar E. Iglesias" <edgar.iglesias@xilinx.com>,
	Markus Armbruster <armbru@redhat.com>,
	Bruce Rogers <brogers@suse.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Marcel Apfelbaum <marcel@redhat.com>,
	Borislav Petkov <bp@suse.de>,
	Thomas Lendacky <Thomas.Lendacky@amd.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Alistair Francis <alistair.francis@xilinx.com>,
	Cornelia Huck <cornelia.huck@de.ibm.com>,
	Richard Henderson <rth@twiddle.net>,
	Peter Crosthwaite <crosthwaite.peter@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v11 21/28] sev/i386: add debug encrypt and decrypt commands
Date: Wed, 7 Mar 2018 17:27:04 +0000	[thread overview]
Message-ID: <20180307172703.GH3089@work-vm> (raw)
In-Reply-To: <20180307165038.88640-22-brijesh.singh@amd.com>

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> KVM_SEV_DBG_DECRYPT and KVM_SEV_DBG_ENCRYPT commands are used for
> decrypting and encrypting guest memory region. The command works only if
> the guest policy allows the debugging.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  accel/kvm/kvm-all.c      |  1 +
>  stubs/sev.c              |  4 ++++
>  target/i386/sev.c        | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
>  target/i386/trace-events |  1 +
>  4 files changed, 63 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 411aa87719e6..8089173491dd 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1682,6 +1682,7 @@ static int kvm_init(MachineState *ms)
>          }
>  
>          kvm_state->memcrypt_encrypt_data = sev_encrypt_data;
> +        kvm_state->memcrypt_debug_ops = sev_set_debug_ops;
>      }
>  
>      ret = kvm_arch_init(ms, s);
> diff --git a/stubs/sev.c b/stubs/sev.c
> index 2e20f3b73a5b..73f5c7f93a67 100644
> --- a/stubs/sev.c
> +++ b/stubs/sev.c
> @@ -15,6 +15,10 @@
>  #include "qemu-common.h"
>  #include "sysemu/sev.h"
>  
> +void sev_set_debug_ops(void *handle, MemoryRegion *mr)
> +{
> +}
> +
>  int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
>  {
>      return 1;
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 5fbb3105ccd4..bcfc09558c8c 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -24,6 +24,7 @@
>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
>  
>  static SEVState *sev_state;
> +static MemoryRegionRAMReadWriteOps  sev_ops;
>  
>  static const char *const sev_fw_errlist[] = {
>      "",
> @@ -601,6 +602,46 @@ sev_vm_state_change(void *opaque, int running, RunState state)
>      }
>  }
>  
> +static int
> +sev_dbg_enc_dec(uint8_t *dst, const uint8_t *src, uint32_t len, bool write)
> +{
> +    int ret, error;
> +    struct kvm_sev_dbg dbg;
> +
> +    dbg.src_uaddr = (unsigned long)src;
> +    dbg.dst_uaddr = (unsigned long)dst;
> +    dbg.len = len;
> +
> +    trace_kvm_sev_debug(write ? "encrypt" : "decrypt", src, dst, len);
> +    ret = sev_ioctl(sev_state->sev_fd,
> +                    write ? KVM_SEV_DBG_ENCRYPT : KVM_SEV_DBG_DECRYPT,
> +                    &dbg, &error);
> +    if (ret) {
> +        error_report("%s (%s) %#llx->%#llx+%#x ret=%d fw_error=%d '%s'",
> +                     __func__, write ? "write" : "read", dbg.src_uaddr,
> +                     dbg.dst_uaddr, dbg.len, ret, error,
> +                     fw_error_to_str(error));
> +    }
> +
> +    return ret;
> +}
> +
> +static int
> +sev_mem_read(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs)
> +{
> +    assert(attrs.debug);
> +
> +    return sev_dbg_enc_dec(dst, src, len, false);
> +}
> +
> +static int
> +sev_mem_write(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs)
> +{
> +    assert(attrs.debug);
> +
> +    return sev_dbg_enc_dec(dst, src, len, true);
> +}
> +
>  void *
>  sev_guest_init(const char *id)
>  {
> @@ -701,6 +742,22 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
>      return 0;
>  }
>  
> +void
> +sev_set_debug_ops(void *handle, MemoryRegion *mr)
> +{
> +    SEVState *s = (SEVState *)handle;
> +
> +    /* If policy does not allow debug then no need to register ops */
> +    if (s->policy & SEV_POLICY_NODBG) {
> +        return;
> +    }

So what happens if someone tries to use a gdb or monitor command when
policy didn't allow debug?  Does it end up with an obvious error
somehow?

Dave

> +    sev_ops.read = sev_mem_read;
> +    sev_ops.write = sev_mem_write;
> +
> +    memory_region_set_ram_debug_ops(mr, &sev_ops);
> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index b1fbde6e40fe..00aa6e98d810 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -15,3 +15,4 @@ kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session
>  kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
>  kvm_sev_launch_measurement(const char *value) "data %s"
>  kvm_sev_launch_finish(void) ""
> +kvm_sev_debug(const char *op, const uint8_t *src, uint8_t *dst, int len) "(%s) src %p dst %p len %d"
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK