| Message ID | 20200528205114.42078-2-tobin@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for SEV Launch Secret Injection | expand |
On Thu, 2020-05-28 at 16:51 -0400, Tobin Feldman-Fitzthum wrote: > --- a/qapi/misc-target.json > +++ b/qapi/misc-target.json > @@ -200,6 +200,26 @@ > { 'command': 'query-sev-capabilities', 'returns': 'SevCapability', > 'if': 'defined(TARGET_I386)' } > > +## > +# @sev-inject-launch-secret: > +# > +# This command injects a secret blob into memory of SEV guest. > +# > +# @packet-header: the launch secret packet header encoded in base64 > +# > +# @secret: the launch secret data to be injected encoded in base64 > +# > +# @gpa: the guest physical address where secret will be injected. > + GPA provided here will be ignored if guest ROM specifies > + the a launch secret GPA. Shouldn't we eliminate the gpa argument to this now the gpa is extracted from OVMF? You add it here but don't take it out in the next patch. > +# Since: 5.0.0 > +# > +## > +{ 'command': 'sev-inject-launch-secret', > + 'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' }, Java (i.e. Json) people hate underscores and abbreviations. I bet they'll want this to be 'packet-header' > + 'if': 'defined(TARGET_I386)' } > + > ## > # @dump-skeys: > # > diff --git a/target/i386/monitor.c b/target/i386/monitor.c > index 27ebfa3ad2..5c2b7d2c17 100644 > --- a/target/i386/monitor.c > +++ b/target/i386/monitor.c > @@ -736,3 +736,11 @@ SevCapability *qmp_query_sev_capabilities(Error > **errp) > > return data; > } > + > +void qmp_sev_inject_launch_secret(const char *packet_hdr, > + const char *secret, uint64_t gpa, > + Error **errp) > +{ > + if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0) > + error_setg(errp, "SEV inject secret failed"); > +} > diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c > index e5ee13309c..2b8c5f1f53 100644 > --- a/target/i386/sev-stub.c > +++ b/target/i386/sev-stub.c > @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void) > { > return NULL; > } > +int sev_inject_launch_secret(const char *hdr, const char *secret, > + uint64_t gpa) > +{ > + return 1; > +} > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 846018a12d..774e47d9d1 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -28,6 +28,7 @@ > #include "sysemu/runstate.h" > #include "trace.h" > #include "migration/blocker.h" > +#include "exec/address-spaces.h" > > #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ > #define DEFAULT_SEV_DEVICE "/dev/sev" > @@ -743,6 +744,88 @@ sev_encrypt_data(void *handle, uint8_t *ptr, > uint64_t len) > return 0; > } > > + > +static void * > +gpa2hva(hwaddr addr, uint64_t size) > +{ > + MemoryRegionSection mrs = > memory_region_find(get_system_memory(), > + addr, size); > + > + if (!mrs.mr) { > + error_report("No memory is mapped at address 0x%" > HWADDR_PRIx, addr); > + return NULL; > + } > + > + if (!memory_region_is_ram(mrs.mr) && > !memory_region_is_romd(mrs.mr)) { > + error_report("Memory at address 0x%" HWADDR_PRIx "is not > RAM", addr); > + memory_region_unref(mrs.mr); > + return NULL; > + } We can still check this, but it should be like an assertion failure. Since the GPA is selected by the OVMF build there should be no way it can't be mapped into the host. [...] > --- a/tests/qtest/qmp-cmd-test.c > +++ b/tests/qtest/qmp-cmd-test.c > @@ -93,10 +93,10 @@ static bool query_is_blacklisted(const char *cmd) > /* Success depends on target-specific build configuration: > */ > "query-pci", /* CONFIG_PCI */ > /* Success depends on launching SEV guest */ > - "query-sev-launch-measure", > + // "query-sev-launch-measure", > /* Success depends on Host or Hypervisor SEV support */ > - "query-sev", > - "query-sev-capabilities", > + // "query-sev", > + // "query-sev-capabilities", We're eliminating existing tests ... is that just a stray hunk that you forgot to remove? James
On 5/28/20 3:51 PM, Tobin Feldman-Fitzthum wrote: > From: Tobin Feldman-Fitzthum <tobin@ibm.com> > > AMD SEV allows a guest owner to inject a secret blob > into the memory of a virtual machine. The secret is > encrypted with the SEV Transport Encryption Key and > integrity is guaranteed with the Transport Integrity > Key. Although QEMU faciliates the injection of the > launch secret, it cannot access the secret. > > Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> > --- > +++ b/qapi/misc-target.json > @@ -200,6 +200,26 @@ > { 'command': 'query-sev-capabilities', 'returns': 'SevCapability', > 'if': 'defined(TARGET_I386)' } > > +## > +# @sev-inject-launch-secret: > +# > +# This command injects a secret blob into memory of SEV guest. > +# > +# @packet-header: the launch secret packet header encoded in base64 > +# > +# @secret: the launch secret data to be injected encoded in base64 > +# > +# @gpa: the guest physical address where secret will be injected. > + GPA provided here will be ignored if guest ROM specifies > + the a launch secret GPA. Missing # on the wrapped lines. > +# > +# Since: 5.0.0 You've missed 5.0, and more sites tend to use x.y instead of x.y.z (although we aren't consistent); this should be 'Since: 5.1' > +# > +## > +{ 'command': 'sev-inject-launch-secret', > + 'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' }, This does not match your documentation above, which named it 'packet-header'. Should 'gpa' be optional, to account for the case where ROM specifies it?
On 2020-05-28 17:42, Eric Blake wrote: > On 5/28/20 3:51 PM, Tobin Feldman-Fitzthum wrote: >> From: Tobin Feldman-Fitzthum <tobin@ibm.com> >> >> AMD SEV allows a guest owner to inject a secret blob >> into the memory of a virtual machine. The secret is >> encrypted with the SEV Transport Encryption Key and >> integrity is guaranteed with the Transport Integrity >> Key. Although QEMU faciliates the injection of the >> launch secret, it cannot access the secret. >> >> Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> >> --- > >> +++ b/qapi/misc-target.json >> @@ -200,6 +200,26 @@ >> { 'command': 'query-sev-capabilities', 'returns': 'SevCapability', >> 'if': 'defined(TARGET_I386)' } >> +## >> +# @sev-inject-launch-secret: >> +# >> +# This command injects a secret blob into memory of SEV guest. >> +# >> +# @packet-header: the launch secret packet header encoded in base64 >> +# >> +# @secret: the launch secret data to be injected encoded in base64 >> +# >> +# @gpa: the guest physical address where secret will be injected. >> + GPA provided here will be ignored if guest ROM specifies >> + the a launch secret GPA. > > Missing # on the wrapped lines. > >> +# >> +# Since: 5.0.0 > > You've missed 5.0, and more sites tend to use x.y instead of x.y.z > (although we aren't consistent); this should be 'Since: 5.1' > >> +# >> +## >> +{ 'command': 'sev-inject-launch-secret', >> + 'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' }, > > This does not match your documentation above, which named it > 'packet-header'. Should 'gpa' be optional, to account for the case > where ROM specifies it? My bad on the syntax issues. I think making GPA optional makes sense. In the first patch we can have it be required and in the second we add the option to scan the ROM.
On 2020-05-28 17:00, James Bottomley wrote: > On Thu, 2020-05-28 at 16:51 -0400, Tobin Feldman-Fitzthum wrote: >> --- a/qapi/misc-target.json >> +++ b/qapi/misc-target.json >> @@ -200,6 +200,26 @@ >> { 'command': 'query-sev-capabilities', 'returns': 'SevCapability', >> 'if': 'defined(TARGET_I386)' } >> >> +## >> +# @sev-inject-launch-secret: >> +# >> +# This command injects a secret blob into memory of SEV guest. >> +# >> +# @packet-header: the launch secret packet header encoded in base64 >> +# >> +# @secret: the launch secret data to be injected encoded in base64 >> +# >> +# @gpa: the guest physical address where secret will be injected. >> + GPA provided here will be ignored if guest ROM specifies >> + the a launch secret GPA. > > Shouldn't we eliminate the gpa argument to this now the gpa is > extracted from OVMF? You add it here but don't take it out in the next > patch. > I think having GPA as an optional argument might make the most sense. Users may or may not know how to use the argument, but it is probably a good idea to give another option besides sticking the GPA into the ROM. >> +# Since: 5.0.0 >> +# >> +## >> +{ 'command': 'sev-inject-launch-secret', >> + 'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' }, > > Java (i.e. Json) people hate underscores and abbreviations. I bet > they'll want this to be 'packet-header' > Happy to change this. >> + 'if': 'defined(TARGET_I386)' } >> + >> ## >> # @dump-skeys: >> # >> diff --git a/target/i386/monitor.c b/target/i386/monitor.c >> index 27ebfa3ad2..5c2b7d2c17 100644 >> --- a/target/i386/monitor.c >> +++ b/target/i386/monitor.c >> @@ -736,3 +736,11 @@ SevCapability *qmp_query_sev_capabilities(Error >> **errp) >> >> return data; >> } >> + >> +void qmp_sev_inject_launch_secret(const char *packet_hdr, >> + const char *secret, uint64_t gpa, >> + Error **errp) >> +{ >> + if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0) >> + error_setg(errp, "SEV inject secret failed"); >> +} >> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c >> index e5ee13309c..2b8c5f1f53 100644 >> --- a/target/i386/sev-stub.c >> +++ b/target/i386/sev-stub.c >> @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void) >> { >> return NULL; >> } >> +int sev_inject_launch_secret(const char *hdr, const char *secret, >> + uint64_t gpa) >> +{ >> + return 1; >> +} >> diff --git a/target/i386/sev.c b/target/i386/sev.c >> index 846018a12d..774e47d9d1 100644 >> --- a/target/i386/sev.c >> +++ b/target/i386/sev.c >> @@ -28,6 +28,7 @@ >> #include "sysemu/runstate.h" >> #include "trace.h" >> #include "migration/blocker.h" >> +#include "exec/address-spaces.h" >> >> #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ >> #define DEFAULT_SEV_DEVICE "/dev/sev" >> @@ -743,6 +744,88 @@ sev_encrypt_data(void *handle, uint8_t *ptr, >> uint64_t len) >> return 0; >> } >> >> + >> +static void * >> +gpa2hva(hwaddr addr, uint64_t size) >> +{ >> + MemoryRegionSection mrs = >> memory_region_find(get_system_memory(), >> + addr, size); >> + >> + if (!mrs.mr) { >> + error_report("No memory is mapped at address 0x%" >> HWADDR_PRIx, addr); >> + return NULL; >> + } >> + >> + if (!memory_region_is_ram(mrs.mr) && >> !memory_region_is_romd(mrs.mr)) { >> + error_report("Memory at address 0x%" HWADDR_PRIx "is not >> RAM", addr); >> + memory_region_unref(mrs.mr); >> + return NULL; >> + } > > We can still check this, but it should be like an assertion failure. > Since the GPA is selected by the OVMF build there should be no way it > can't be mapped into the host. > > [...] >> --- a/tests/qtest/qmp-cmd-test.c >> +++ b/tests/qtest/qmp-cmd-test.c >> @@ -93,10 +93,10 @@ static bool query_is_blacklisted(const char *cmd) >> /* Success depends on target-specific build configuration: >> */ >> "query-pci", /* CONFIG_PCI */ >> /* Success depends on launching SEV guest */ >> - "query-sev-launch-measure", >> + // "query-sev-launch-measure", >> /* Success depends on Host or Hypervisor SEV support */ >> - "query-sev", >> - "query-sev-capabilities", >> + // "query-sev", >> + // "query-sev-capabilities", > > We're eliminating existing tests ... is that just a stray hunk that you > forgot to remove? > Yes. > James
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h index 98c1ec8d38..313ee30fc8 100644 --- a/include/sysemu/sev.h +++ b/include/sysemu/sev.h @@ -18,4 +18,6 @@ void *sev_guest_init(const char *id); int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len); +int sev_inject_launch_secret(const char *hdr, const char *secret, + uint64_t gpa); #endif diff --git a/qapi/misc-target.json b/qapi/misc-target.json index dee3b45930..27458b765b 100644 --- a/qapi/misc-target.json +++ b/qapi/misc-target.json @@ -200,6 +200,26 @@ { 'command': 'query-sev-capabilities', 'returns': 'SevCapability', 'if': 'defined(TARGET_I386)' } +## +# @sev-inject-launch-secret: +# +# This command injects a secret blob into memory of SEV guest. +# +# @packet-header: the launch secret packet header encoded in base64 +# +# @secret: the launch secret data to be injected encoded in base64 +# +# @gpa: the guest physical address where secret will be injected. + GPA provided here will be ignored if guest ROM specifies + the a launch secret GPA. +# +# Since: 5.0.0 +# +## +{ 'command': 'sev-inject-launch-secret', + 'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' }, + 'if': 'defined(TARGET_I386)' } + ## # @dump-skeys: # diff --git a/target/i386/monitor.c b/target/i386/monitor.c index 27ebfa3ad2..5c2b7d2c17 100644 --- a/target/i386/monitor.c +++ b/target/i386/monitor.c @@ -736,3 +736,11 @@ SevCapability *qmp_query_sev_capabilities(Error **errp) return data; } + +void qmp_sev_inject_launch_secret(const char *packet_hdr, + const char *secret, uint64_t gpa, + Error **errp) +{ + if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0) + error_setg(errp, "SEV inject secret failed"); +} diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c index e5ee13309c..2b8c5f1f53 100644 --- a/target/i386/sev-stub.c +++ b/target/i386/sev-stub.c @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void) { return NULL; } +int sev_inject_launch_secret(const char *hdr, const char *secret, + uint64_t gpa) +{ + return 1; +} diff --git a/target/i386/sev.c b/target/i386/sev.c index 846018a12d..774e47d9d1 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -28,6 +28,7 @@ #include "sysemu/runstate.h" #include "trace.h" #include "migration/blocker.h" +#include "exec/address-spaces.h" #define DEFAULT_GUEST_POLICY 0x1 /* disable debug */ #define DEFAULT_SEV_DEVICE "/dev/sev" @@ -743,6 +744,88 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len) return 0; } + +static void * +gpa2hva(hwaddr addr, uint64_t size) +{ + MemoryRegionSection mrs = memory_region_find(get_system_memory(), + addr, size); + + if (!mrs.mr) { + error_report("No memory is mapped at address 0x%" HWADDR_PRIx, addr); + return NULL; + } + + if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) { + error_report("Memory at address 0x%" HWADDR_PRIx "is not RAM", addr); + memory_region_unref(mrs.mr); + return NULL; + } + + return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region); +} + +int sev_inject_launch_secret(const char *packet_hdr, + const char *secret, uint64_t gpa) +{ + struct kvm_sev_launch_secret *input = NULL; + guchar *data = NULL, *hdr = NULL; + int error, ret = 1; + void *hva; + gsize hdr_sz = 0, data_sz = 0; + + /* secret can be inject only in this state */ + if (!sev_check_state(SEV_STATE_LAUNCH_SECRET)) { + error_report("Not in correct state. %x",sev_state->state); + return 1; + } + + hdr = g_base64_decode(packet_hdr, &hdr_sz); + if (!hdr || !hdr_sz) { + error_report("SEV: Failed to decode sequence header"); + return 1; + } + + data = g_base64_decode(secret, &data_sz); + if (!data || !data_sz) { + error_report("SEV: Failed to decode data"); + goto err; + } + + hva = gpa2hva(gpa, data_sz); + if (!hva) { + goto err; + } + input = g_new0(struct kvm_sev_launch_secret, 1); + + input->hdr_uaddr = (unsigned long)hdr; + input->hdr_len = hdr_sz; + + input->trans_uaddr = (unsigned long)data; + input->trans_len = data_sz; + + input->guest_uaddr = (unsigned long)hva; + input->guest_len = data_sz; + + trace_kvm_sev_launch_secret(gpa, input->guest_uaddr, + input->trans_uaddr, input->trans_len); + + ret = sev_ioctl(sev_state->sev_fd,KVM_SEV_LAUNCH_SECRET, input, &error); + if (ret) { + error_report("SEV: failed to inject secret ret=%d fw_error=%d '%s'", + ret, error, fw_error_to_str(error)); + goto err; + } + + ret = 0; + +err: + g_free(data); + g_free(hdr); + g_free(input); + return ret; +} + static void sev_register_types(void) { diff --git a/target/i386/trace-events b/target/i386/trace-events index 789c700d4a..9f299e94a2 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_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d" diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c index 9f5228cd99..50b2b42830 100644 --- a/tests/qtest/qmp-cmd-test.c +++ b/tests/qtest/qmp-cmd-test.c @@ -93,10 +93,10 @@ static bool query_is_blacklisted(const char *cmd) /* Success depends on target-specific build configuration: */ "query-pci", /* CONFIG_PCI */ /* Success depends on launching SEV guest */ - "query-sev-launch-measure", + // "query-sev-launch-measure", /* Success depends on Host or Hypervisor SEV support */ - "query-sev", - "query-sev-capabilities", + // "query-sev", + // "query-sev-capabilities", NULL }; int i;