[PATCH v3 0/2] sev: enable secret injection to a self described area in OVMF

[PATCH v3 0/2] sev: enable secret injection to a self described area in OVMF

[James Bottomley]

v3: add bounds checking and remove misleading comment

v2: fix build issues and update comments.  Also drop the first patch
    which is now merged upstream

The 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

 hw/i386/pc_sysfw.c    | 112 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h  |   4 ++
 qapi/misc-target.json |   2 +-
 target/i386/monitor.c |  23 ++++++++-
 4 files changed, 139 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH v3 1/2] 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>

---

v2: fix brace warnings and return values
v3: add bounds checking for flash tables
---
 hw/i386/pc_sysfw.c   | 112 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h |   4 ++
 2 files changed, 116 insertions(+)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 92e90ff013..8ef73dbc3a 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -124,6 +124,113 @@ 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, size_t flash_size)
+{
+    uint8_t *ptr;
+    QemuUUID guid;
+    int tot_len;
+
+    /* should only be called once */
+    if (ovmf_table) {
+        return;
+    }
+
+    if (flash_size < TARGET_PAGE_SIZE) {
+        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 >= sizeof(QemuUUID) + sizeof(uint16_t)) {
+        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 in
+         * the zero case
+         */
+        if (len < sizeof(QemuUUID) + sizeof(uint16_t)) {
+            return false;
+        } else if (len > tot_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 - sizeof(QemuUUID) - sizeof(uint16_t);
+            }
+            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,
@@ -195,6 +302,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 2aa8797c6e..19a53f745f 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"
@@ -188,6 +189,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 v3 2/2] 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>

---

v2: fix line length warning, add more comments about sev area
v2: remove misleading comment
---
 qapi/misc-target.json |  2 +-
 target/i386/monitor.c | 23 ++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 06ef8757f0..0c7491cd82 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..5994408bee 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,29 @@ 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 v3 1/2] pc: add parser for OVMF reset block

[Dr. David Alan Gilbert]

* James Bottomley (jejb@linux.ibm.com) wrote:
> 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>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> ---
> 
> v2: fix brace warnings and return values
> v3: add bounds checking for flash tables
> ---
>  hw/i386/pc_sysfw.c   | 112 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h |   4 ++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 92e90ff013..8ef73dbc3a 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -124,6 +124,113 @@ 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, size_t flash_size)
> +{
> +    uint8_t *ptr;
> +    QemuUUID guid;
> +    int tot_len;
> +
> +    /* should only be called once */
> +    if (ovmf_table) {
> +        return;
> +    }
> +
> +    if (flash_size < TARGET_PAGE_SIZE) {
> +        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 >= sizeof(QemuUUID) + sizeof(uint16_t)) {
> +        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 in
> +         * the zero case
> +         */
> +        if (len < sizeof(QemuUUID) + sizeof(uint16_t)) {
> +            return false;
> +        } else if (len > tot_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 - sizeof(QemuUUID) - sizeof(uint16_t);
> +            }
> +            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,
> @@ -195,6 +302,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 2aa8797c6e..19a53f745f 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"
> @@ -188,6 +189,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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* James Bottomley (jejb@linux.ibm.com) 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>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> 
> ---
> 
> v2: fix line length warning, add more comments about sev area
> v2: remove misleading comment
> ---
>  qapi/misc-target.json |  2 +-
>  target/i386/monitor.c | 23 ++++++++++++++++++++++-
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 06ef8757f0..0c7491cd82 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..5994408bee 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,29 @@ 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Daniel P. Berrangé]

On Thu, Feb 04, 2021 at 11:39:39AM -0800, 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>
> 
> ---
> 
> v2: fix line length warning, add more comments about sev area
> v2: remove misleading comment
> ---
>  qapi/misc-target.json |  2 +-
>  target/i386/monitor.c | 23 ++++++++++++++++++++++-
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 06ef8757f0..0c7491cd82 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..5994408bee 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,29 @@ 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;
> +        }

IIUC, historically QEMU has gone out of its way to avoid creating a
direct dependancy on specific firmware implementation details such
as this, so this whole approach makes me feel really uneasy.

We have the fw_cfg interface which lets QEMU put well known data items
at specific places in memory, which the firmware can then access and
do sometime with.   That all happens at startup though before CPUs
are running, so this situation at runtime is more complex.

None the less I still wonder if we can take a better approach such that
the firmware explicitly tells us this location to use, rather than QEMU
parsing data tables to reverse engineer it.


> +        area = (struct sev_secret_area *)data;
> +        gpa = area->base;
> +    }
> +
>      sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
>  }
> -- 
> 2.26.2
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Paolo Bonzini]

On 05/02/21 10:51, Daniel P. Berrangé wrote:
>> +        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;
>> +        }
> IIUC, historically QEMU has gone out of its way to avoid creating a
> direct dependancy on specific firmware implementation details such
> as this, so this whole approach makes me feel really uneasy.

The problem here is that this secret must be measured and therefore 
cannot be extracted by the guest out of fw_cfg.  Note that there's no 
reason why other firmware than OVMF could not adopt the same interface.

Paolo

Re: [PATCH v3 0/2] sev: enable secret injection to a self described area in OVMF

[Paolo Bonzini]

On 04/02/21 20:39, James Bottomley wrote:
> v3: add bounds checking and remove misleading comment
> 
> v2: fix build issues and update comments.  Also drop the first patch
>      which is now merged upstream
> 
> The 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
> 
>   hw/i386/pc_sysfw.c    | 112 ++++++++++++++++++++++++++++++++++++++++++
>   include/hw/i386/pc.h  |   4 ++
>   qapi/misc-target.json |   2 +-
>   target/i386/monitor.c |  23 ++++++++-
>   4 files changed, 139 insertions(+), 2 deletions(-)
> 

Queued, thanks.

Paolo

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

[Daniel P. Berrangé]

On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote:
> On 05/02/21 10:51, Daniel P. Berrangé wrote:
> > > +        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;
> > > +        }
> > IIUC, historically QEMU has gone out of its way to avoid creating a
> > direct dependancy on specific firmware implementation details such
> > as this, so this whole approach makes me feel really uneasy.
> 
> The problem here is that this secret must be measured and therefore cannot
> be extracted by the guest out of fw_cfg.  Note that there's no reason why
> other firmware than OVMF could not adopt the same interface.

I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a
way for OVMF to tell QEMU where to store it


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Paolo Bonzini]

On 05/02/21 12:37, Daniel P. Berrangé wrote:
> On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote:
>> On 05/02/21 10:51, Daniel P. Berrangé wrote:
>>>> +        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;
>>>> +        }
>>> IIUC, historically QEMU has gone out of its way to avoid creating a
>>> direct dependancy on specific firmware implementation details such
>>> as this, so this whole approach makes me feel really uneasy.
>>
>> The problem here is that this secret must be measured and therefore cannot
>> be extracted by the guest out of fw_cfg.  Note that there's no reason why
>> other firmware than OVMF could not adopt the same interface.
> 
> I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a
> way for OVMF to tell QEMU where to store it

I may be misunderstanding, but I think QEMU has to store it before OVMF 
runs, because the measurement is "sealed" when the VM starts.

Paolo

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

[Daniel P. Berrangé]

On Fri, Feb 05, 2021 at 12:45:18PM +0100, Paolo Bonzini wrote:
> On 05/02/21 12:37, Daniel P. Berrangé wrote:
> > On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote:
> > > On 05/02/21 10:51, Daniel P. Berrangé wrote:
> > > > > +        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;
> > > > > +        }
> > > > IIUC, historically QEMU has gone out of its way to avoid creating a
> > > > direct dependancy on specific firmware implementation details such
> > > > as this, so this whole approach makes me feel really uneasy.
> > > 
> > > The problem here is that this secret must be measured and therefore cannot
> > > be extracted by the guest out of fw_cfg.  Note that there's no reason why
> > > other firmware than OVMF could not adopt the same interface.
> > 
> > I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a
> > way for OVMF to tell QEMU where to store it
> 
> I may be misunderstanding, but I think QEMU has to store it before OVMF
> runs, because the measurement is "sealed" when the VM starts.

Oh, right, I see what you mean

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 05/02/21 12:37, Daniel P. Berrangé wrote:
> > On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote:
> > > On 05/02/21 10:51, Daniel P. Berrangé wrote:
> > > > > +        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;
> > > > > +        }
> > > > IIUC, historically QEMU has gone out of its way to avoid creating a
> > > > direct dependancy on specific firmware implementation details such
> > > > as this, so this whole approach makes me feel really uneasy.
> > > 
> > > The problem here is that this secret must be measured and therefore cannot
> > > be extracted by the guest out of fw_cfg.  Note that there's no reason why
> > > other firmware than OVMF could not adopt the same interface.
> > 
> > I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a
> > way for OVMF to tell QEMU where to store it
> 
> I may be misunderstanding, but I think QEMU has to store it before OVMF
> runs, because the measurement is "sealed" when the VM starts.

Yes, that's correct, it's a feature of SEV and SEV-ES that the flow is:

  * measure data
  * attest
  * insert some encrypted data
  * Start execution

I would agree this code is pretty much tied to OVMF's weird GUID based
system when it's not needed; but there's no standard to follow here.

Dave

> Paolo
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK