[PATCH 0/3] sev: enable seret injection to a self described area in OVMF

[PATCH 0/3] sev: enable seret injection to a self described area in OVMF

[James Bottomley]

This patch series includes one from Tobin that has already been posted
and reviewed:

https://lore.kernel.org/qemu-devel/20201027170303.47550-1-tobin@linux.ibm.com/

I'm adding it here because it's a required precursor, but it can be
dropped from this series if it's already being upstreamed elsewhere.

The remaining two patches introduce a parser for the optional OVMF
description table which is placed just below the reset vector (the
format of the table is described in the patch itself) and also adds a
hook to pull out the description of the SEV secret area location and
use it in place of the sev-inject-launch-secret gpa.

James

---

James Bottomley (2):
  pc: add parser for OVMF reset block
  sev: update sev-inject-launch-secret to make gpa optional

Tobin Feldman-Fitzthum (1):
  sev: add sev-inject-launch-secret

 hw/i386/pc_sysfw.c        | 95 +++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h      |  4 ++
 include/monitor/monitor.h |  3 ++
 include/sysemu/sev.h      |  2 +
 monitor/misc.c            | 17 +++++--
 qapi/misc-target.json     | 18 ++++++++
 target/i386/monitor.c     | 27 +++++++++++
 target/i386/sev-stub.c    |  5 +++
 target/i386/sev.c         | 65 +++++++++++++++++++++++++++
 target/i386/trace-events  |  1 +
 10 files changed, 233 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH 1/3] sev: add sev-inject-launch-secret

[James Bottomley]

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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 include/monitor/monitor.h |  3 ++
 include/sysemu/sev.h      |  2 ++
 monitor/misc.c            | 17 +++++++---
 qapi/misc-target.json     | 18 +++++++++++
 target/i386/monitor.c     |  7 +++++
 target/i386/sev-stub.c    |  5 +++
 target/i386/sev.c         | 65 +++++++++++++++++++++++++++++++++++++++
 target/i386/trace-events  |  1 +
 8 files changed, 114 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 398211a034..4e40f7c850 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -667,10 +667,11 @@ 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)
 {
+    Int128 gpa_region_size;
     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);
@@ -683,6 +684,14 @@ static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
         return NULL;
     }
 
+    gpa_region_size = int128_make64(size);
+    if (int128_lt(mrs.size, gpa_region_size)) {
+        error_setg(errp, "Size of memory region at 0x%" HWADDR_PRIx
+                   " exceeded.", addr);
+        memory_region_unref(mrs.mr);
+        return NULL;
+    }
+
     *p_mr = mrs.mr;
     return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
 }
@@ -694,7 +703,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 +779,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 9f9e1c42f4..1bc91442b1 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -729,3 +729,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..c1fecc2101 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, Error **errp)
+{
+    return 1;
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 93c4d60b82..1546606811 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,69 @@ 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;
+
+    if (!sev_guest) {
+        error_setg(errp, "SEV: SEV not enabled.");
+        return 1;
+    }
+
+    /* 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;
+    }
+
+    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"
-- 
2.26.2

[PATCH 2/3] pc: add parser for OVMF reset block

[James Bottomley]

OVMF is developing a mechanism for depositing a GUIDed table just
below the known location of the reset vector.  The table goes
backwards in memory so all entries are of the form

<data>|len|<GUID>

Where <data> is arbtrary size and type, <len> is a uint16_t and
describes the entire length of the entry from the beginning of the
data to the end of the guid.

The foot of the table is of this form and <len> for this case
describes the entire size of the table.  The table foot GUID is
defined by OVMF as 96b582de-1fb2-45f7-baea-a366c55a082d and if the
table is present this GUID is just below the reset vector, 48 bytes
before the end of the firmware file.

Add a parser for the ovmf reset block which takes a copy of the block,
if the table foot guid is found, minus the footer and a function for
later traversal to return the data area of any specified GUIDs.

Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 hw/i386/pc_sysfw.c   | 95 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h |  4 ++
 2 files changed, 99 insertions(+)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index b6c0822fe3..bd8a16d620 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -133,6 +133,96 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms)
     }
 }
 
+#define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d"
+
+static uint8_t *ovmf_table;
+static int ovmf_table_len;
+
+static void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, int flash_size)
+{
+    uint8_t *ptr;
+    QemuUUID guid;
+    int tot_len;
+
+    /* should only be called once */
+    if (ovmf_table)
+        return;
+
+    /*
+     * if this is OVMF there will be a table footer
+     * guid 48 bytes before the end of the flash file.  If it's
+     * not found, silently abort the flash parsing.
+     */
+    qemu_uuid_parse(OVMF_TABLE_FOOTER_GUID, &guid);
+    guid = qemu_uuid_bswap(guid); /* guids are LE */
+    ptr = flash_ptr + flash_size - 48;
+    if (!qemu_uuid_is_equal((QemuUUID *)ptr, &guid))
+        return;
+
+    /* if found, just before is two byte table length */
+    ptr -= sizeof(uint16_t);
+    tot_len = le16_to_cpu(*(uint16_t *)ptr) - sizeof(guid) - sizeof(uint16_t);
+
+    if (tot_len <= 0)
+        return;
+
+    ovmf_table = g_malloc(tot_len);
+    ovmf_table_len = tot_len;
+
+    /*
+     * ptr is the foot of the table, so copy it all to the newly
+     * allocated ovmf_table and then set the ovmf_table pointer
+     * to the table foot
+     */
+    memcpy(ovmf_table, ptr - tot_len, tot_len);
+    ovmf_table += tot_len;
+}
+
+bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
+                               int *data_len)
+{
+    uint8_t *ptr = ovmf_table;
+    int tot_len = ovmf_table_len;
+    QemuUUID entry_guid;
+
+    if (qemu_uuid_parse(entry, &entry_guid) < 0)
+        return false;
+
+    if (!ptr)
+        return false;
+
+    entry_guid = qemu_uuid_bswap(entry_guid); /* guids are LE */
+    while (tot_len > 0) {
+        int len;
+        QemuUUID *guid;
+
+        /*
+         * The data structure is
+         *   arbitrary length data
+         *   2 byte length of entire entry
+         *   16 byte guid
+         */
+        guid = (QemuUUID *)(ptr - sizeof(QemuUUID));
+        len = le16_to_cpu(*(uint16_t *)(ptr - sizeof(QemuUUID) -
+                                        sizeof(uint16_t)));
+
+        /* just in case the table is corrupt, wouldn't want to spin */
+        if (!len)
+                return false;
+
+        ptr -= len;
+        tot_len -= len;
+        if (qemu_uuid_is_equal(guid, &entry_guid)) {
+            if (data)
+                *data = ptr;
+            if (data_len)
+                *data_len = len;
+            return true;
+        }
+    }
+    return false;
+}
+
 /*
  * Map the pcms->flash[] from 4GiB downward, and realize.
  * Map them in descending order, i.e. pcms->flash[0] at the top,
@@ -204,6 +294,11 @@ static void pc_system_flash_map(PCMachineState *pcms,
             if (kvm_memcrypt_enabled()) {
                 flash_ptr = memory_region_get_ram_ptr(flash_mem);
                 flash_size = memory_region_size(flash_mem);
+                /*
+                 * OVMF places a GUIDed structures in the flash, so
+                 * search for them
+                 */
+                pc_system_parse_ovmf_flash(flash_ptr, flash_size);
                 ret = kvm_memcrypt_encrypt_data(flash_ptr, flash_size);
                 if (ret) {
                     error_report("failed to encrypt pflash rom");
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 911e460097..a8c97fe822 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -3,6 +3,7 @@
 
 #include "qemu/notify.h"
 #include "qapi/qapi-types-common.h"
+#include "qemu/uuid.h"
 #include "hw/boards.h"
 #include "hw/block/fdc.h"
 #include "hw/block/flash.h"
@@ -186,6 +187,9 @@ ISADevice *pc_find_fdc0(void);
 void pc_system_flash_create(PCMachineState *pcms);
 void pc_system_flash_cleanup_unused(PCMachineState *pcms);
 void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
+bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
+                               int *data_len);
+
 
 /* acpi-build.c */
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-- 
2.26.2

[PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional

[James Bottomley]

If the gpa isn't specified, it's value is extracted from the OVMF
properties table located below the reset vector (and if this doesn't
exist, an error is returned).  OVMF has defined the GUID for the SEV
secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format of
the <data> is: <base>|<size> where both are uint32_t.  We extract
<base> and use it as the gpa for the injection.

Note: it is expected that the injected secret will also be GUID
described but since qemu can't interpret it, the format is left
undefined here.

Signed-off-by: James Bottomley <jejb@linux.ibm.com>
---
 qapi/misc-target.json |  2 +-
 target/i386/monitor.c | 22 +++++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4486a543ae..1ee4e62f85 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -216,7 +216,7 @@
 #
 ##
 { 'command': 'sev-inject-launch-secret',
-  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' },
+  'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
   'if': 'defined(TARGET_I386)' }
 
 ##
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 1bc91442b1..a99e3dd2b3 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -34,6 +34,7 @@
 #include "sev_i386.h"
 #include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qapi-commands-misc.h"
+#include "hw/i386/pc.h"
 
 /* Perform linear address sign extension */
 static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
@@ -730,9 +731,28 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
     return sev_get_capabilities(errp);
 }
 
+#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
+struct sev_secret_area {
+    uint32_t base;
+    uint32_t size;
+};
+
 void qmp_sev_inject_launch_secret(const char *packet_hdr,
-                                  const char *secret, uint64_t gpa,
+                                  const char *secret,
+                                  bool has_gpa, uint64_t gpa,
                                   Error **errp)
 {
+    if (!has_gpa) {
+        uint8_t *data;
+        struct sev_secret_area *area;
+
+        if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) {
+            error_setg(errp, "SEV: no secret area found in OVMF, gpa must be specified.");
+            return;
+        }
+        area = (struct sev_secret_area *)data;
+        gpa = area->base;
+    }
+
     sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
 }
-- 
2.26.2

Re: [PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional

[Tom Lendacky]

On 12/9/20 11:23 AM, James Bottomley wrote:
> If the gpa isn't specified, it's value is extracted from the OVMF
> properties table located below the reset vector (and if this doesn't
> exist, an error is returned).  OVMF has defined the GUID for the SEV
> secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format of
> the <data> is: <base>|<size> where both are uint32_t.  We extract
> <base> and use it as the gpa for the injection.
> 
> Note: it is expected that the injected secret will also be GUID
> described but since qemu can't interpret it, the format is left
> undefined here.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> ---
>   qapi/misc-target.json |  2 +-
>   target/i386/monitor.c | 22 +++++++++++++++++++++-
>   2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 4486a543ae..1ee4e62f85 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -216,7 +216,7 @@
>   #
>   ##
>   { 'command': 'sev-inject-launch-secret',
> -  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' },
> +  'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
>     'if': 'defined(TARGET_I386)' }
>   
>   ##
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 1bc91442b1..a99e3dd2b3 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -34,6 +34,7 @@
>   #include "sev_i386.h"
>   #include "qapi/qapi-commands-misc-target.h"
>   #include "qapi/qapi-commands-misc.h"
> +#include "hw/i386/pc.h"
>   
>   /* Perform linear address sign extension */
>   static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> @@ -730,9 +731,28 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
>       return sev_get_capabilities(errp);
>   }
>   
> +#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
> +struct sev_secret_area {
> +    uint32_t base;
> +    uint32_t size;
> +};
> +

Originally, the idea was to allow expanding of these GUID based structures 
by pre-pending data to them, but based on how pc_system_ovmf_table_find() 
returns the pointer to the start of the structure (based on the length 
found in the structure), I believe that expansion could be done by 
appending to the structure, which seems more logical. For example, if this 
structure is ever expanded, it can use the third parameter of 
pc_system_ovmf_table_find() to get the length and compare that to the size 
of the structure to determine if new version of the structure is present 
in the firmware.

Otherwise you can't do the nice easy assignment below:
   area = (struct sev_secret_area *)data;

You actually have to do some math:
   area = (struct sev_secret_area *)(data + data_len -
                                     sizeof(QemuUUID) - sizeof(uint16_t) -
				    sizeof(*area));

or add the QemuUUID and uint16_t fields to sev_secret_area and:
   area = (struct sev_secret_area *)(data + data_len - sizeof(*area));

Or we make the decision that these GUID structs should never change, just 
add a new one to the table if more info is needed.

Whatever we decide should probably be documented in both the OVMF patches 
and the Qemu patches.

Thanks,
Tom

>   void qmp_sev_inject_launch_secret(const char *packet_hdr,
> -                                  const char *secret, uint64_t gpa,
> +                                  const char *secret,
> +                                  bool has_gpa, uint64_t gpa,
>                                     Error **errp)
>   {
> +    if (!has_gpa) {
> +        uint8_t *data;
> +        struct sev_secret_area *area;
> +
> +        if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) {
> +            error_setg(errp, "SEV: no secret area found in OVMF, gpa must be specified.");
> +            return;
> +        }
> +        area = (struct sev_secret_area *)data;
> +        gpa = area->base;
> +    }
> +
>       sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
>   }
> 

Re: [PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional

[James Bottomley]

On Fri, 2020-12-11 at 16:00 -0600, Tom Lendacky wrote:
> On 12/9/20 11:23 AM, James Bottomley wrote:
> > If the gpa isn't specified, it's value is extracted from the OVMF
> > properties table located below the reset vector (and if this
> > doesn't
> > exist, an error is returned).  OVMF has defined the GUID for the
> > SEV
> > secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format
> > of
> > the <data> is: <base>|<size> where both are uint32_t.  We extract
> > <base> and use it as the gpa for the injection.
> > 
> > Note: it is expected that the injected secret will also be GUID
> > described but since qemu can't interpret it, the format is left
> > undefined here.
> > 
> > Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> > ---
> >   qapi/misc-target.json |  2 +-
> >   target/i386/monitor.c | 22 +++++++++++++++++++++-
> >   2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > index 4486a543ae..1ee4e62f85 100644
> > --- a/qapi/misc-target.json
> > +++ b/qapi/misc-target.json
> > @@ -216,7 +216,7 @@
> >   #
> >   ##
> >   { 'command': 'sev-inject-launch-secret',
> > -  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa':
> > 'uint64' },
> > +  'data': { 'packet-header': 'str', 'secret': 'str', '*gpa':
> > 'uint64' },
> >     'if': 'defined(TARGET_I386)' }
> >   
> >   ##
> > diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> > index 1bc91442b1..a99e3dd2b3 100644
> > --- a/target/i386/monitor.c
> > +++ b/target/i386/monitor.c
> > @@ -34,6 +34,7 @@
> >   #include "sev_i386.h"
> >   #include "qapi/qapi-commands-misc-target.h"
> >   #include "qapi/qapi-commands-misc.h"
> > +#include "hw/i386/pc.h"
> >   
> >   /* Perform linear address sign extension */
> >   static hwaddr addr_canonical(CPUArchState *env, hwaddr addr)
> > @@ -730,9 +731,28 @@ SevCapability
> > *qmp_query_sev_capabilities(Error **errp)
> >       return sev_get_capabilities(errp);
> >   }
> >   
> > +#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294"
> > +struct sev_secret_area {
> > +    uint32_t base;
> > +    uint32_t size;
> > +};
> > +
> 
> Originally, the idea was to allow expanding of these GUID based
> structures by pre-pending data to them, but based on how
> pc_system_ovmf_table_find() returns the pointer to the start of the
> structure (based on the length found in the structure), I believe
> that expansion could be done by appending to the structure, which
> seems more logical. For example, if this structure is ever expanded,
> it can use the third parameter of pc_system_ovmf_table_find() to get
> the length and compare that to the size  of the structure to
> determine if new version of the structure is present in the firmware.

Actually, I don't think it much matters.  It looks like the len it
would return is wrong ... it should be the length of just the returned
data pointer (without the length or guid), so ptr+len would point to
the foot of the data if that's what you want.

> Otherwise you can't do the nice easy assignment below:
>    area = (struct sev_secret_area *)data;
> 
> You actually have to do some math:
>    area = (struct sev_secret_area *)(data + data_len -
>                                      sizeof(QemuUUID) -
> sizeof(uint16_t) -
> 				    sizeof(*area));
> 
> or add the QemuUUID and uint16_t fields to sev_secret_area and:
>    area = (struct sev_secret_area *)(data + data_len -
> sizeof(*area));

Right, that's why I think patch 2/3 should do

    *data_len = len - sizeof(QemuUUID) - sizeof(uint16_t)

> Or we make the decision that these GUID structs should never change,
> just add a new one to the table if more info is needed.

Actually, the fact that the only guid the table depends on is the table
footer GUID, you can always remove guids and add new ones.  So I think
it's up to whoever is using the GUID to decide the policy.

So for this one I'm not checking the length, which argues it wouldn't
be subject to the added length new data rule and I'd have to use a new
guid for new information.  However, I could also see situations where
you would check the length and thus would have the ability to add
fields (either at the beginning or the end).

> Whatever we decide should probably be documented in both the OVMF
> patches and the Qemu patches.

OK, I can add a comment about my use case and you can add one
documenting your length based use case.

James

Re: [PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional

[Tom Lendacky]

On 12/11/20 4:45 PM, James Bottomley wrote:
> On Fri, 2020-12-11 at 16:00 -0600, Tom Lendacky wrote:
>> On 12/9/20 11:23 AM, James Bottomley wrote:
> 
> So for this one I'm not checking the length, which argues it wouldn't
> be subject to the added length new data rule and I'd have to use a new
> guid for new information.  However, I could also see situations where
> you would check the length and thus would have the ability to add
> fields (either at the beginning or the end).

I think this paragraph explains it nicely and a slightly expanded comment 
with this information would be enough.

Thanks,
Tom


> 
>> Whatever we decide should probably be documented in both the OVMF
>> patches and the Qemu patches.
> 
> OK, I can add a comment about my use case and you can add one
> documenting your length based use case.
> 
> James
> 
> 

Re: [PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional

[Laszlo Ersek]

On 12/11/20 23:54, Tom Lendacky wrote:
> On 12/11/20 4:45 PM, James Bottomley wrote:
>> On Fri, 2020-12-11 at 16:00 -0600, Tom Lendacky wrote:
>>> On 12/9/20 11:23 AM, James Bottomley wrote:
>>
>> So for this one I'm not checking the length, which argues it wouldn't
>> be subject to the added length new data rule and I'd have to use a new
>> guid for new information.  However, I could also see situations where
>> you would check the length and thus would have the ability to add
>> fields (either at the beginning or the end).
> 
> I think this paragraph explains it nicely and a slightly expanded
> comment with this information would be enough.

Sounds good to me as well, thank you.

Laszlo

Re: [PATCH 0/3] sev: enable seret injection to a self described area in OVMF

[James Bottomley]

On Wed, 2020-12-09 at 09:23 -0800, James Bottomley wrote:
> This patch series includes one from Tobin that has already been
> posted
> and reviewed:
> 
> https://lore.kernel.org/qemu-devel/20201027170303.47550-1-tobin@linux.ibm.com/
> 
> I'm adding it here because it's a required precursor, but it can be
> dropped from this series if it's already being upstreamed elsewhere.
> 
> The remaining two patches introduce a parser for the optional OVMF
> description table which is placed just below the reset vector (the
> format of the table is described in the patch itself) and also adds a
> hook to pull out the description of the SEV secret area location and
> use it in place of the sev-inject-launch-secret gpa.

For those who want to try this at home (assuming you have a SEV capable
AMD system), you need the amd sev-tool:

https://github.com/AMDESE/sev-tool/

To build the launch bundle (which contains the TIK and TEK key pair). 
Once you have that, you need to pass in both the launch bundle and the
-S flag to get QEMU to pause before starting the VM to allow
measurement and secret injection.  I'm using the python script below to
interact with the paused VM, verify the measurement, inject the secret
and resume the VM.  The below python script uses qmp.py from the qemu
git repository, so you'll have to adjust your path to it.

James

---

#!/usr/bin/python3
##
# Python script to inject a secret disk password into a paused SEV VM
#  (to pause the VM start with -S option)
#
# This assumes you've already created the launch bundle using sev-tool
# from https://github.com/AMDESE/sev-tool.git
#
# sev-tool --generate_launch_blob
#
# creates several files, the only one this script needs is the TIK and TEK
# keys which are stored in tmp_tk.bin
#
# Once TIK/TEK are known, the script will probe the VM for the sev
# parameters needed to calculate the launch measure, retrieve the launch
# measure and verify against the measure calculated from the OVMF hash
# and if that matches create the secret bundle and inject it
#
# Tables and chapters refer to the amd 55766.pdf document
#
# https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
##
import sys
import os 
import base64
import hmac
import hashlib
from argparse import ArgumentParser
from uuid import UUID
from Crypto.Cipher import AES
from Crypto.Util import Counter
from git.qemu.python.qemu import qmp

if __name__ == "__main__":
    parser = ArgumentParser(description='Inject secret into SEV')
    parser.add_argument('--tiktek-file',
                        help='file where sev-tool stored the TIK/TEK combination, defaults to tmp_tk.bin',
                        default='tmp_tk.bin')
    parser.add_argument('--passwd',
                        help='Disk Password',
                        required=True)
    parser.add_argument('--ovmf-hash',
                        help='hash of OVMF firmware blob in hex')
    parser.add_argument('--ovmf-file',
                        help='location of OVMF file to calculate hash from')
    parser.add_argument('--socket',
                        help='Socket to connect to QMP on, defaults to localhost:6550',
                        default='localhost:6550')
    args = parser.parse_args()

    if (args.ovmf_file):
        fh = open (args.ovmf_file, 'rb')
        h = hashlib.sha256(fh.read())
        ovmf_hash = h.digest()
    elif (args.ovmf_hash):
        ovmf_hash = bytearray.fromhex(args.ovmf_hash)
    else:
        parser.error('one of --ovmf-hash or -ovmf-file must be specified')

    if (args.socket[0] == '/'):
        socket = args.socket
    elif (':' in args.socket):
        s = args.socket.split(':')
        socket = (s[0], int(s[1]))
    else:
        parse.error('--socket must be <host>:<port> or /path/to/unix')

    fh=open(args.tiktek_file, 'rb')
    tiktek=bytearray(fh.read())
    fh.close()

    ##
    #  tiktek file is just two binary aes128 keys
    ##
    TEK=tiktek[0:16]
    TIK=tiktek[16:32]

    disk_secret = args.passwd

    Qmp = qmp.QEMUMonitorProtocol(address=socket);
    Qmp.connect()
    caps = Qmp.command('query-sev')
    print('SEV query found API={api-major}.{api-minor} build={build-id} policy={policy}'.format(**caps))
    h = hmac.new(TIK, digestmod='sha256');

    ##
    # calculated per section 6.5.2
    ##
    h.update(bytes([0x04]))
    h.update(caps['api-major'].to_bytes(1,byteorder='little'))
    h.update(caps['api-minor'].to_bytes(1,byteorder='little'))
    h.update(caps['build-id'].to_bytes(1,byteorder='little'))
    h.update(caps['policy'].to_bytes(4,byteorder='little'))
    h.update(ovmf_hash)

    print('\nGetting Launch Measurement')
    meas = Qmp.command('query-sev-launch-measure')
    launch_measure = base64.b64decode(meas['data'])

    ##
    # returned data per Table 52. LAUNCH_MEASURE Measurement Buffer
    ##
    nonce = launch_measure[32:48]
    h.update(nonce)
    measure = launch_measure[0:32]

    print('Measure:   ', measure.hex())
    print('should be: ', h.digest().hex())
    print('')

    if (measure != h.digest()):
        sys.exit('Measurement doesn\'t match')

    print('Measurement matches, Injecting Secret')

    ##
    # construct the secret table: two guids + 4 byte lengths plus string
    # and zero terminator
    #
    # Secret layout is  guid, len (4 bytes), data
    # with len being the length from start of guid to end of data
    #
    # The table header covers the entire table then each entry covers
    # only its local data
    #
    # our current table has the header guid with total table length
    # followed by the secret guid with the zero terminated secret 
    ##
    
    # total length of table: header plus one entry with trailing \0
    l = 16 + 4 + 16 + 4 + len(disk_secret) + 1
    secret = bytearray(l);
    secret[0:16] = UUID('{1e74f542-71dd-4d66-963e-ef4287ff173b}').bytes_le
    secret[16:20] = len(secret).to_bytes(4, byteorder='little')
    secret[20:36] = UUID('{736869e5-84f0-4973-92ec-06879ce3da0b}').bytes_le
    secret[36:40] = (16 + 4 + len(disk_secret) + 1).to_bytes(4, byteorder='little')
    secret[40:40+len(disk_secret)] = disk_secret.encode()
    
    ##
    # encrypt the secret table with the TEK in ctr mode using a random IV
    ##
    IV=os.urandom(16)
    # -EKNUCKLEHEADS in python crypto don't understand CTR mode
    e = AES.new(TEK, AES.MODE_CTR, counter=Counter.new(128,initial_value=int.from_bytes(IV, byteorder='big')));
    encrypted_secret = e.encrypt(bytes(secret))

    ##
    # ultimately needs to be an argument, but there's only
    # compressed and no real use case
    ##
    FLAGS = 0

    ##
    # Table 55. LAUNCH_SECRET Packet Header Buffer
    ##
    header=bytearray(52);
    header[0:4]=FLAGS.to_bytes(4,byteorder='little')
    header[4:20]=IV
    h = hmac.new(TIK, digestmod='sha256');
    h.update(bytes([0x01]))
    # FLAGS || IV
    h.update(header[0:20])
    h.update(l.to_bytes(4, byteorder='little'))
    h.update(l.to_bytes(4, byteorder='little'))
    h.update(encrypted_secret)
    h.update(measure)
    header[20:52]=h.digest()

    Qmp.command('sev-inject-launch-secret',
                **{'packet-header': base64.b64encode(header).decode(),
                   'secret': base64.b64encode(encrypted_secret).decode()
                })

    print('\nSecret Injection Successful, starting VM')

    Qmp.command('cont')

Re: [PATCH 0/3] sev: enable seret injection to a self described area in OVMF

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201209172334.14100-1-jejb@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201209172334.14100-1-jejb@linux.ibm.com
Subject: [PATCH 0/3] sev: enable seret injection to a self described area in OVMF

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20201209172334.14100-1-jejb@linux.ibm.com -> patchew/20201209172334.14100-1-jejb@linux.ibm.com
 * [new tag]         patchew/20201209173536.1437351-1-groug@kaod.org -> patchew/20201209173536.1437351-1-groug@kaod.org
 - [tag update]      patchew/cover.1607467819.git.alistair.francis@wdc.com -> patchew/cover.1607467819.git.alistair.francis@wdc.com
Switched to a new branch 'test'
f51e90d sev: update sev-inject-launch-secret to make gpa optional
4f524bc pc: add parser for OVMF reset block
915ed52 sev: add sev-inject-launch-secret

=== OUTPUT BEGIN ===
1/3 Checking commit 915ed52b80aa (sev: add sev-inject-launch-secret)
2/3 Checking commit 4f524bc2ee18 (pc: add parser for OVMF reset block)
ERROR: braces {} are necessary for all arms of this statement
#50: FILE: hw/i386/pc_sysfw.c:148:
+    if (ovmf_table)
[...]

ERROR: braces {} are necessary for all arms of this statement
#61: FILE: hw/i386/pc_sysfw.c:159:
+    if (!qemu_uuid_is_equal((QemuUUID *)ptr, &guid))
[...]

ERROR: braces {} are necessary for all arms of this statement
#68: FILE: hw/i386/pc_sysfw.c:166:
+    if (tot_len <= 0)
[...]

ERROR: braces {} are necessary for all arms of this statement
#90: FILE: hw/i386/pc_sysfw.c:188:
+    if (qemu_uuid_parse(entry, &entry_guid) < 0)
[...]

ERROR: braces {} are necessary for all arms of this statement
#93: FILE: hw/i386/pc_sysfw.c:191:
+    if (!ptr)
[...]

ERROR: braces {} are necessary for all arms of this statement
#112: FILE: hw/i386/pc_sysfw.c:210:
+        if (!len)
[...]

ERROR: braces {} are necessary for all arms of this statement
#118: FILE: hw/i386/pc_sysfw.c:216:
+            if (data)
[...]

ERROR: braces {} are necessary for all arms of this statement
#120: FILE: hw/i386/pc_sysfw.c:218:
+            if (data_len)
[...]

total: 8 errors, 0 warnings, 123 lines checked

Patch 2/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/3 Checking commit f51e90dbc914 (sev: update sev-inject-launch-secret to make gpa optional)
WARNING: line over 80 characters
#67: FILE: target/i386/monitor.c:750:
+            error_setg(errp, "SEV: no secret area found in OVMF, gpa must be specified.");

total: 0 errors, 1 warnings, 44 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201209172334.14100-1-jejb@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com