Re: [PATCH v4] sev: add sev-inject-launch-secret

[Tobin Feldman-Fitzthum <tobin@linux.ibm.com>]

On 2020-10-14 11:42, Brijesh Singh wrote:
> On 10/14/20 10:17 AM, tobin@linux.ibm.com wrote:
>> From: Tobin Feldman-Fitzthum <tobin@linux.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 facilitates the injection of the
>> launch secret, it cannot access the secret.
>> 
>> Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>> ---
>>  include/monitor/monitor.h |  3 ++
>>  include/sysemu/sev.h      |  2 ++
>>  monitor/misc.c            |  8 +++---
>>  qapi/misc-target.json     | 18 ++++++++++++
>>  target/i386/monitor.c     |  7 +++++
>>  target/i386/sev-stub.c    |  5 ++++
>>  target/i386/sev.c         | 60 
>> +++++++++++++++++++++++++++++++++++++++
>>  target/i386/trace-events  |  1 +
>>  8 files changed, 100 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> index 348bfad3d5..af3887bb71 100644
>> --- a/include/monitor/monitor.h
>> +++ b/include/monitor/monitor.h
>> @@ -4,6 +4,7 @@
>>  #include "block/block.h"
>>  #include "qapi/qapi-types-misc.h"
>>  #include "qemu/readline.h"
>> +#include "include/exec/hwaddr.h"
>> 
>>  typedef struct MonitorHMP MonitorHMP;
>>  typedef struct MonitorOptions MonitorOptions;
>> @@ -37,6 +38,8 @@ void monitor_flush(Monitor *mon);
>>  int monitor_set_cpu(Monitor *mon, int cpu_index);
>>  int monitor_get_cpu_index(Monitor *mon);
>> 
>> +void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error 
>> **errp);
>> +
>>  void monitor_read_command(MonitorHMP *mon, int show_prompt);
>>  int monitor_read_password(MonitorHMP *mon, ReadLineFunc 
>> *readline_func,
>>                            void *opaque);
>> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
>> index 98c1ec8d38..7ab6e3e31d 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, Error **errp);
>>  #endif
>> diff --git a/monitor/misc.c b/monitor/misc.c
>> index 4a859fb24a..f1ade245d5 100644
>> --- a/monitor/misc.c
>> +++ b/monitor/misc.c
>> @@ -667,10 +667,10 @@ static void hmp_physical_memory_dump(Monitor 
>> *mon, const QDict *qdict)
>>      memory_dump(mon, count, format, size, addr, 1);
>>  }
>> 
>> -static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
>> +void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error 
>> **errp)
>>  {
>>      MemoryRegionSection mrs = memory_region_find(get_system_memory(),
>> -                                                 addr, 1);
>> +                                                 addr, size);
>> 
>>      if (!mrs.mr) {
>>          error_setg(errp, "No memory is mapped at address 0x%" 
>> HWADDR_PRIx, addr);
>> @@ -694,7 +694,7 @@ static void hmp_gpa2hva(Monitor *mon, const QDict 
>> *qdict)
>>      MemoryRegion *mr = NULL;
>>      void *ptr;
>> 
>> -    ptr = gpa2hva(&mr, addr, &local_err);
>> +    ptr = gpa2hva(&mr, addr, 1, &local_err);
>>      if (local_err) {
>>          error_report_err(local_err);
>>          return;
>> @@ -770,7 +770,7 @@ static void hmp_gpa2hpa(Monitor *mon, const QDict 
>> *qdict)
>>      void *ptr;
>>      uint64_t physaddr;
>> 
>> -    ptr = gpa2hva(&mr, addr, &local_err);
>> +    ptr = gpa2hva(&mr, addr, 1, &local_err);
>>      if (local_err) {
>>          error_report_err(local_err);
>>          return;
>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>> index 1e561fa97b..4486a543ae 100644
>> --- a/qapi/misc-target.json
>> +++ b/qapi/misc-target.json
>> @@ -201,6 +201,24 @@
>>  { '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.
>> +#
>> +# Since: 5.2
>> +#
>> +##
>> +{ 'command': 'sev-inject-launch-secret',
>> +  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' 
>> },
>> +  'if': 'defined(TARGET_I386)' }
>> +
>>  ##
>>  # @dump-skeys:
>>  #
>> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
>> index 7abae3c8df..f9d4951465 100644
>> --- a/target/i386/monitor.c
>> +++ b/target/i386/monitor.c
>> @@ -728,3 +728,10 @@ SevCapability *qmp_query_sev_capabilities(Error 
>> **errp)
>>  {
>>      return sev_get_capabilities(errp);
>>  }
>> +
>> +void qmp_sev_inject_launch_secret(const char *packet_hdr,
>> +                                  const char *secret, uint64_t gpa,
>> +                                  Error **errp)
>> +{
>> +    sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
>> +}
>> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
>> index 88e3f39a1e..2d2ee54cc6 100644
>> --- a/target/i386/sev-stub.c
>> +++ b/target/i386/sev-stub.c
>> @@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)
>>      error_setg(errp, "SEV is not available in this QEMU");
>>      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 93c4d60b82..5275a624b3 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -29,6 +29,8 @@
>>  #include "trace.h"
>>  #include "migration/blocker.h"
>>  #include "qom/object.h"
>> +#include "exec/address-spaces.h"
>> +#include "monitor/monitor.h"
>> 
>>  #define TYPE_SEV_GUEST "sev-guest"
>>  OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
>> @@ -785,6 +787,64 @@ sev_encrypt_data(void *handle, uint8_t *ptr, 
>> uint64_t len)
>>      return 0;
>>  }
>> 
>> +int sev_inject_launch_secret(const char *packet_hdr, const char 
>> *secret,
>> +                             uint64_t gpa, Error **errp)
>> +{
>> +    struct kvm_sev_launch_secret input;
>> +    g_autofree guchar *data = NULL, *hdr = NULL;
>> +    int error, ret = 1;
>> +    void *hva;
>> +    gsize hdr_sz = 0, data_sz = 0;
>> +    MemoryRegion *mr = NULL;
>> +
>> +    /* secret can be injected only in this state */
>> +    if (!sev_check_state(sev_guest, SEV_STATE_LAUNCH_SECRET)) {
>> +        error_setg(errp, "SEV: Not in correct state. (LSECRET) %x",
>> +                     sev_guest->state);
>> +        return 1;
>> +    }
> 
> 
> Should we be checking the sev_guest variable is assigned before the
> state check ? This will prevent an assert() if user calls the
> sev_inject_launch_secret on non-SEV guest.
> 
> 
Probably a good idea.
>> +
>> +    hdr = g_base64_decode(packet_hdr, &hdr_sz);
>> +    if (!hdr || !hdr_sz) {
>> +        error_setg(errp, "SEV: Failed to decode sequence header");
>> +        return 1;
>> +    }
>> +
>> +    data = g_base64_decode(secret, &data_sz);
>> +    if (!data || !data_sz) {
>> +        error_setg(errp, "SEV: Failed to decode data");
>> +        return 1;
>> +    }
>> +
>> +    hva = gpa2hva(&mr, gpa, data_sz, errp);
>> +    if (!hva) {
>> +        error_prepend(errp, "SEV: Failed to calculate guest address: 
>> ");
>> +        return 1;
>> +    }
>> +
>> +    input.hdr_uaddr = (uint64_t)(unsigned long)hdr;
>> +    input.hdr_len = hdr_sz;
>> +
>> +    input.trans_uaddr = (uint64_t)(unsigned long)data;
>> +    input.trans_len = data_sz;
>> +
>> +    input.guest_uaddr = (uint64_t)(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_guest->sev_fd, KVM_SEV_LAUNCH_SECRET,
>> +                    &input, &error);
>> +    if (ret) {
>> +        error_setg(errp, "SEV: failed to inject secret ret=%d 
>> fw_error=%d '%s'",
>> +                     ret, error, fw_error_to_str(error));
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  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"