| Message ID | 20180307165038.88640-22-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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
On 03/07/2018 11:27 AM, Dr. David Alan Gilbert wrote: [...] >> +{ >> + 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? > In those cases caller will get encrypted bytes, leading to unintelligible data. It can sometime translate into obvious errors e.g caller tries to walk guest pagtable and it gets garbage and will have trouble dumping the pgtables etc. Many times qemu calls ldphys_* functions to access the data it may get tricky to report the errors. -Brijesh
* Brijesh Singh (brijesh.singh@amd.com) wrote: > > > On 03/07/2018 11:27 AM, Dr. David Alan Gilbert wrote: > > [...] > > > > +{ > > > + 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? > > > > In those cases caller will get encrypted bytes, leading to unintelligible > data. It can sometime translate into obvious errors e.g caller tries to > walk guest pagtable and it gets garbage and will have trouble dumping the > pgtables etc. Many times qemu calls ldphys_* functions to access the data it > may get tricky to report the errors. So would it make sense to have something like: sev_mem_cant_read(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs) { error_report("SEV Guest policy does not allow debug access"); return -EPERM; } 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) { sev_ops.read = sev_mem_cant_read; sev_ops.write = sev_mem_cant_write; } else { sev_ops.read = sev_mem_read; sev_ops.write = sev_mem_write; } Dave > > -Brijesh -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 3/7/18 12:24 PM, Dr. David Alan Gilbert wrote: > * Brijesh Singh (brijesh.singh@amd.com) wrote: >> >> On 03/07/2018 11:27 AM, Dr. David Alan Gilbert wrote: >> >> [...] >> >>>> +{ >>>> + 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? >>> >> In those cases caller will get encrypted bytes, leading to unintelligible >> data. It can sometime translate into obvious errors e.g caller tries to >> walk guest pagtable and it gets garbage and will have trouble dumping the >> pgtables etc. Many times qemu calls ldphys_* functions to access the data it >> may get tricky to report the errors. > So would it make sense to have something like: > > sev_mem_cant_read(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs) > { > error_report("SEV Guest policy does not allow debug access"); > > return -EPERM; > } In very early patches we had something similar but I was not sure if that was right thing. Any debug accesses were printing ton of messages and also in some case caller actually wants to dump the memory content (e.g x /10gx 0x000).. what we should return in those cases ? In my approach was if debug was not enabled then simply don't decrypt the memory and provide the raw data. There was some discussion to have very high level security policy which may have attribute like debug=on|off, if debug is disabled then QEMU monitor can display messages like debug not allowed to better inform user. > 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) { > sev_ops.read = sev_mem_cant_read; > sev_ops.write = sev_mem_cant_write; > } else { > sev_ops.read = sev_mem_read; > sev_ops.write = sev_mem_write; > } > > Dave > >> -Brijesh > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Brijesh Singh (brijesh.singh@amd.com) wrote: > > > On 3/7/18 12:24 PM, Dr. David Alan Gilbert wrote: > > * Brijesh Singh (brijesh.singh@amd.com) wrote: > >> > >> On 03/07/2018 11:27 AM, Dr. David Alan Gilbert wrote: > >> > >> [...] > >> > >>>> +{ > >>>> + 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? > >>> > >> In those cases caller will get encrypted bytes, leading to unintelligible > >> data. It can sometime translate into obvious errors e.g caller tries to > >> walk guest pagtable and it gets garbage and will have trouble dumping the > >> pgtables etc. Many times qemu calls ldphys_* functions to access the data it > >> may get tricky to report the errors. > > So would it make sense to have something like: > > > > sev_mem_cant_read(uint8_t *dst, const uint8_t *src, uint32_t len, MemTxAttrs attrs) > > { > > error_report("SEV Guest policy does not allow debug access"); > > > > return -EPERM; > > } > > In very early patches we had something similar but I was not sure if > that was right thing. Any debug accesses were printing ton of messages OK, if it would generate silly amounts of debug then leave it as is; but I bet it'll confuse someone in the future when they try and dig through it for debug without realising SEV-debug is there! Dave > and also in some case caller actually wants to dump the memory content > (e.g x /10gx 0x000).. what we should return in those cases ? In my > approach was if debug was not enabled then simply don't decrypt the > memory and provide the raw data. > > There was some discussion to have very high level security policy which > may have attribute like debug=on|off, if debug is disabled then QEMU > monitor can display messages like debug not allowed to better inform user. > > > > 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) { > > sev_ops.read = sev_mem_cant_read; > > sev_ops.write = sev_mem_cant_write; > > } else { > > sev_ops.read = sev_mem_read; > > sev_ops.write = sev_mem_write; > > } > > > > Dave > > > >> -Brijesh > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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; + } + + 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"
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(+)