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

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

[James Bottomley]

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    | 106 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h  |   4 ++
 qapi/misc-target.json |   2 +-
 target/i386/monitor.c |  27 ++++++++++-
 4 files changed, 137 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH v2 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
---
 hw/i386/pc_sysfw.c   | 106 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h |   4 ++
 2 files changed, 110 insertions(+)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 92e90ff013..436b78c587 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -124,6 +124,107 @@ 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 in
+         * the zero case
+         */
+        if (len < sizeof(QemuUUID) + sizeof(uint16_t)) {
+                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 +296,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

Re: [PATCH v2 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>
> 
> ---
> 
> v2: fix brace warnings and return values
> ---
>  hw/i386/pc_sysfw.c   | 106 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h |   4 ++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 92e90ff013..436b78c587 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -124,6 +124,107 @@ 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)

Maybe size_t for 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;

I think since flash_size is coming from memory_region_size it's probably
rounded to a page size by now, but perhaps we should always check we
have enough space before we start moving pointers around

(Given that the OVMF binary might be provided by the guest owner, we
have to consider it might be a vector to attack the hypervisor).

> +    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)));

Again I think we need to be checking tot_len > (sizeof(QemuUUID) +
sizeof(uint16_t)) before doing this dereference.

> +        /*
> +         * 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;
> +        }
> +
> +        ptr -= len;
> +        tot_len -= len;

and that len is smaller or equal to tot_len here.

> +        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 +296,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 v2 1/2] pc: add parser for OVMF reset block

[James Bottomley]

On Tue, 2021-01-26 at 12:22 +0000, Dr. David Alan Gilbert wrote:
> * 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>
> > 
> > ---
> > 
> > v2: fix brace warnings and return values
> > ---
> >  hw/i386/pc_sysfw.c   | 106
> > +++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/i386/pc.h |   4 ++
> >  2 files changed, 110 insertions(+)
> > 
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index 92e90ff013..436b78c587 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -124,6 +124,107 @@ 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)
> 
> Maybe size_t for flash_size?

Heh, sure, who knows how big OVMF will get ...  but I get the point
about an int overflow attack.

> > +{
> > +    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;
> 
> I think since flash_size is coming from memory_region_size it's
> probably rounded to a page size by now, but perhaps we should always
> check we have enough space before we start moving pointers around

I think OVMF must be at least a page, so I can add that check.

> (Given that the OVMF binary might be provided by the guest owner, we
> have to consider it might be a vector to attack the hypervisor).
> 
> > +    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)));
> 
> Again I think we need to be checking tot_len > (sizeof(QemuUUID) +
> sizeof(uint16_t)) before doing this dereference.

I can make the loop start

  while (tot_len > 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;
> > +        }
> > +
> > +        ptr -= len;
> > +        tot_len -= len;
> 
> and that len is smaller or equal to tot_len here.

OK.

Re: [PATCH v2 1/2] pc: add parser for OVMF reset block

[Dr. David Alan Gilbert]

* James Bottomley (jejb@linux.ibm.com) wrote:
> On Tue, 2021-01-26 at 12:22 +0000, Dr. David Alan Gilbert wrote:
> > * 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>
> > > 
> > > ---
> > > 
> > > v2: fix brace warnings and return values
> > > ---
> > >  hw/i386/pc_sysfw.c   | 106
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/i386/pc.h |   4 ++
> > >  2 files changed, 110 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > > index 92e90ff013..436b78c587 100644
> > > --- a/hw/i386/pc_sysfw.c
> > > +++ b/hw/i386/pc_sysfw.c
> > > @@ -124,6 +124,107 @@ 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)
> > 
> > Maybe size_t for flash_size?
> 
> Heh, sure, who knows how big OVMF will get ...  but I get the point
> about an int overflow attack.

To be honest I was more style than actually worrying about overflow; I
like size_t for sizes.

> > > +{
> > > +    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;
> > 
> > I think since flash_size is coming from memory_region_size it's
> > probably rounded to a page size by now, but perhaps we should always
> > check we have enough space before we start moving pointers around
> 
> I think OVMF must be at least a page, so I can add that check.
> 
> > (Given that the OVMF binary might be provided by the guest owner, we
> > have to consider it might be a vector to attack the hypervisor).
> > 
> > > +    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)));
> > 
> > Again I think we need to be checking tot_len > (sizeof(QemuUUID) +
> > sizeof(uint16_t)) before doing this dereference.
> 
> I can make the loop start
> 
>   while (tot_len > sizeof(QemuUUID) + sizeof(uint16_t))

Yep.

Dave

> > > +        /*
> > > +         * 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;
> > > +        }
> > > +
> > > +        ptr -= len;
> > > +        tot_len -= len;
> > 
> > and that len is smaller or equal to tot_len here.
> 
> OK.
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v2 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
---
 qapi/misc-target.json |  2 +-
 target/i386/monitor.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 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..11bdb04155 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,33 @@ 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;
+
+        /*
+         * not checking length means that this area can't be versioned
+         * by length and would have to be replaced if updated
+         */
+        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 v2 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>
> 
> ---
> 
> v2: fix line length warning, add more comments about sev area
> ---
>  qapi/misc-target.json |  2 +-
>  target/i386/monitor.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 27 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..11bdb04155 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,33 @@ 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;
> +
> +        /*
> +         * not checking length means that this area can't be versioned
> +         * by length and would have to be replaced if updated
> +         */

Can you just explain that a bit more?

> +        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);

Other than me not understanding that comment, I think we're fine:


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

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

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

[James Bottomley]

On Tue, 2021-01-26 at 12:32 +0000, Dr. David Alan Gilbert wrote:
> * 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>
> > 
> > ---
> > 
> > v2: fix line length warning, add more comments about sev area
> > ---
> >  qapi/misc-target.json |  2 +-
> >  target/i386/monitor.c | 27 ++++++++++++++++++++++++++-
> >  2 files changed, 27 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..11bdb04155 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,33 @@ 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;
> > +
> > +        /*
> > +         * not checking length means that this area can't be
> > versioned
> > +         * by length and would have to be replaced if updated
> > +         */
> 
> Can you just explain that a bit more?

It's referring back to the original concept that the reset vector
length would tell you what version of the thing you were using.  So if
you were looking for a property at offset 10 and the length came in as
8 the version was too early.  If it was 18 you had a later version and
your property was present.

The current scheme uses guids which can be versioned by length if you
think you'll add extra properties to them.  This one I don't think
would ever get an extra property, so there's no point checking the
length.  Not checking the length means if I'm wrong and we do need an
extra property it will have to be attached to a new guid.

That's a bit confusing to add to the comment ... how about I just leave
out the comment entirely?

> > +        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);
> 
> Other than me not understanding that comment, I think we're fine:

Thanks.

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

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

[Dr. David Alan Gilbert]

* James Bottomley (jejb@linux.ibm.com) wrote:
> On Tue, 2021-01-26 at 12:32 +0000, Dr. David Alan Gilbert wrote:
> > * 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>
> > > 
> > > ---
> > > 
> > > v2: fix line length warning, add more comments about sev area
> > > ---
> > >  qapi/misc-target.json |  2 +-
> > >  target/i386/monitor.c | 27 ++++++++++++++++++++++++++-
> > >  2 files changed, 27 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..11bdb04155 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,33 @@ 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;
> > > +
> > > +        /*
> > > +         * not checking length means that this area can't be
> > > versioned
> > > +         * by length and would have to be replaced if updated
> > > +         */
> > 
> > Can you just explain that a bit more?
> 
> It's referring back to the original concept that the reset vector
> length would tell you what version of the thing you were using.  So if
> you were looking for a property at offset 10 and the length came in as
> 8 the version was too early.  If it was 18 you had a later version and
> your property was present.
> 
> The current scheme uses guids which can be versioned by length if you
> think you'll add extra properties to them.  This one I don't think
> would ever get an extra property, so there's no point checking the
> length.  Not checking the length means if I'm wrong and we do need an
> extra property it will have to be attached to a new guid.
> 
> That's a bit confusing to add to the comment ... how about I just leave
> out the comment entirely?

Yes, I don't think it makes much sense unless you knew the history.

Dave

> > > +        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);
> > 
> > Other than me not understanding that comment, I think we're fine:
> 
> Thanks.
> 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > >  }
> > > -- 
> > > 2.26.2
> > > 
> > > 
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK