[PATCH v2 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline

[PATCH v2 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline

[Dov Murik]

Currently booting with -kernel/-initrd/-append is not supported in SEV
confidential guests, because the content of these blobs is not measured
and therefore not trusted by the SEV guest.

However, in some cases the kernel, initrd, and cmdline are not secret
but should not be modified by the host.  In such a case, we want to
verify inside the trusted VM that the kernel, initrd, and cmdline are
indeed the ones expected by the Guest Owner, and only if that is the
case go on and boot them up (removing the need for grub inside OVMF in
that mode).

To support that, OVMF adds a special area for hashes of
kernel/initrd/cmdline; that area is expected to be filled by QEMU and
encrypted as part of the initial SEV guest launch.  This in turn makes
the hashes part of the PSP measured content, and OVMF can trust these
inputs if they match the hashes.

This series adds an SEV function to generate the table of hashes for
OVMF and encrypt it (patch 1/2), and calls this function if SEV is
enabled when the kernel/initrd/cmdline are prepared (patch 2/2).

Corresponding OVMF support was submitted to edk2-devel [1] (patch series
"Measured SEV boot with kernel/initrd/cmdline"); it's still under
review.

[1] https://edk2.groups.io/g/devel/topic/patch_v1_0_8_measured_sev/83074450

---

v1: https://lore.kernel.org/qemu-devel/20210525065931.1628554-1-dovmurik@linux.ibm.com/

v2:
 - Extract main functionality to sev.c (with empty stub in sev-stub.c)
 - Use sev_enabled() instead of machine->cgs->ready to detect SEV guest
 - Coding style changes


Dov Murik (2):
  sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux
    boot
  x86/sev: generate SEV kernel loader hashes in x86_load_linux

 hw/i386/x86.c          |  25 ++++++++-
 target/i386/sev-stub.c |   5 ++
 target/i386/sev.c      | 121 +++++++++++++++++++++++++++++++++++++++++
 target/i386/sev_i386.h |  12 ++++
 4 files changed, 162 insertions(+), 1 deletion(-)


base-commit: e4bfa6cd68e0b19f42c0c4ef26c024d39ebab044
-- 
2.25.1

[PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

Add the sev_add_kernel_loader_hashes function to calculate the hashes of
the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
table area.  For this to work, OVMF must support an encrypted area to
place the data which is advertised via a special GUID in the OVMF reset
table.

The hashes of each of the files is calculated (or the string in the case
of the cmdline with trailing '\0' included).  Each entry in the hashes
table is GUID identified and since they're passed through the
sev_encrypt_flash interface, the hashes will be accumulated by the PSP
measurement (SEV_LAUNCH_MEASURE).

Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 target/i386/sev-stub.c |   5 ++
 target/i386/sev.c      | 121 +++++++++++++++++++++++++++++++++++++++++
 target/i386/sev_i386.h |  12 ++++
 3 files changed, 138 insertions(+)

diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 0227cb5177..2b5e42d644 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -81,3 +81,8 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
     error_setg(errp, "SEV is not available in this QEMU");
     return NULL;
 }
+
+bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
+{
+    return false;
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 83df8c09f6..8e3f601bb6 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -23,6 +23,7 @@
 #include "qemu/base64.h"
 #include "qemu/module.h"
 #include "qemu/uuid.h"
+#include "crypto/hash.h"
 #include "sysemu/kvm.h"
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
@@ -83,6 +84,29 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
     uint32_t reset_addr;
 } SevInfoBlock;
 
+#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
+typedef struct __attribute__((__packed__)) SevHashTableDescriptor {
+    /* SEV hash table area guest address */
+    uint32_t base;
+    /* SEV hash table area size (in bytes) */
+    uint32_t size;
+} SevHashTableDescriptor;
+
+/* hard code sha256 digest size */
+#define HASH_SIZE 32
+
+typedef struct __attribute__((__packed__)) SevHashTableEntry {
+    uint8_t guid[16];
+    uint16_t len;
+    uint8_t hash[HASH_SIZE];
+} SevHashTableEntry;
+
+typedef struct __attribute__((__packed__)) SevHashTable {
+    uint8_t guid[16];
+    uint16_t len;
+    SevHashTableEntry entries[];
+} SevHashTable;
+
 static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
@@ -1077,6 +1101,103 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
     return 0;
 }
 
+static const uint8_t sev_hash_table_header_guid[] =
+        UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
+                0xd4, 0x11, 0xfd, 0x21);
+
+static const uint8_t sev_kernel_entry_guid[] =
+        UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
+                0x72, 0xd2, 0x04, 0x5b);
+static const uint8_t sev_initrd_entry_guid[] =
+        UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
+                0x91, 0x69, 0x78, 0x1d);
+static const uint8_t sev_cmdline_entry_guid[] =
+        UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
+                0x4d, 0x36, 0xab, 0x2a);
+
+static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t *guid,
+                                      const uint8_t *hash, size_t hash_len)
+{
+    memcpy(e->guid, guid, sizeof(e->guid));
+    e->len = sizeof(*e);
+    memcpy(e->hash, hash, hash_len);
+}
+
+/*
+ * Add the hashes of the linux kernel/initrd/cmdline to an encrypted guest page
+ * which is included in SEV's initial memory measurement.
+ */
+bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
+{
+    uint8_t *data;
+    SevHashTableDescriptor *area;
+    SevHashTable *ht;
+    SevHashTableEntry *e;
+    uint8_t hash_buf[HASH_SIZE];
+    uint8_t *hash = hash_buf;
+    size_t hash_len = sizeof(hash_buf);
+    int ht_index = 0;
+    int aligned_len;
+
+    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
+        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
+        return false;
+    }
+    area = (SevHashTableDescriptor *)data;
+
+    ht = qemu_map_ram_ptr(NULL, area->base);
+
+    /* Populate the hashes table header */
+    memcpy(ht->guid, sev_hash_table_header_guid, sizeof(ht->guid));
+    ht->len = sizeof(*ht);
+
+    /* Calculate hash of kernel command-line */
+    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
+                           ctx->cmdline_size,
+                           &hash, &hash_len, errp) < 0) {
+        return false;
+    }
+    e = &ht->entries[ht_index++];
+    fill_sev_hash_table_entry(e, sev_cmdline_entry_guid, hash, hash_len);
+
+    /* Calculate hash of initrd */
+    if (ctx->initrd_data) {
+        if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
+                               ctx->initrd_size, &hash, &hash_len, errp) < 0) {
+            return false;
+        }
+        e = &ht->entries[ht_index++];
+        fill_sev_hash_table_entry(e, sev_initrd_entry_guid, hash, hash_len);
+    }
+
+    /* Calculate hash of the kernel */
+    struct iovec iov[2] = {
+        { .iov_base = ctx->setup_data, .iov_len = ctx->setup_size },
+        { .iov_base = ctx->kernel_data, .iov_len = ctx->kernel_size }
+    };
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, 2,
+                            &hash, &hash_len, errp) < 0) {
+        return false;
+    }
+    e = &ht->entries[ht_index++];
+    fill_sev_hash_table_entry(e, sev_kernel_entry_guid, hash, hash_len);
+
+    /* now we have all the possible entries, finalize the hashes table */
+    ht->len += ht_index * sizeof(*e);
+    /* SEV len has to be 16 byte aligned */
+    aligned_len = ROUND_UP(ht->len, 16);
+    if (aligned_len != ht->len) {
+        /* zero the excess data so the measurement can be reliably calculated */
+        memset(&ht->entries[ht_index], 0, aligned_len - ht->len);
+    }
+
+    if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
+        return false;
+    }
+
+    return true;
+}
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index ae6d840478..deb3eec409 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -28,6 +28,17 @@
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
+typedef struct KernelLoaderContext {
+    char *setup_data;
+    size_t setup_size;
+    char *kernel_data;
+    size_t kernel_size;
+    char *initrd_data;
+    size_t initrd_size;
+    char *cmdline_data;
+    size_t cmdline_size;
+} KernelLoaderContext;
+
 extern bool sev_es_enabled(void);
 extern uint64_t sev_get_me_mask(void);
 extern SevInfo *sev_get_info(void);
@@ -37,5 +48,6 @@ extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(Error **errp);
 extern SevAttestationReport *
 sev_get_attestation_report(const char *mnonce, Error **errp);
+extern bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp);
 
 #endif
-- 
2.25.1

[PATCH v2 2/2] x86/sev: generate SEV kernel loader hashes in x86_load_linux

[Dov Murik]

If SEV is enabled and a kernel is passed via -kernel, pass the hashes of
kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV
measured boot.

Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 hw/i386/x86.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index ed796fe6ba..5c46463d9f 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -45,6 +45,7 @@
 #include "hw/i386/fw_cfg.h"
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
+#include "target/i386/sev_i386.h"
 
 #include "hw/acpi/cpu_hotplug.h"
 #include "hw/irq.h"
@@ -778,6 +779,7 @@ void x86_load_linux(X86MachineState *x86ms,
     const char *initrd_filename = machine->initrd_filename;
     const char *dtb_filename = machine->dtb;
     const char *kernel_cmdline = machine->kernel_cmdline;
+    KernelLoaderContext kernel_loader_context = {};
 
     /* Align to 16 bytes as a paranoia measure */
     cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -924,6 +926,8 @@ void x86_load_linux(X86MachineState *x86ms,
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
     fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
+    kernel_loader_context.cmdline_data = (char *)kernel_cmdline;
+    kernel_loader_context.cmdline_size = strlen(kernel_cmdline) + 1;
 
     if (protocol >= 0x202) {
         stl_p(header + 0x228, cmdline_addr);
@@ -1005,6 +1009,8 @@ void x86_load_linux(X86MachineState *x86ms,
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr);
         fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size);
         fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data, initrd_size);
+        kernel_loader_context.initrd_data = initrd_data;
+        kernel_loader_context.initrd_size = initrd_size;
 
         stl_p(header + 0x218, initrd_addr);
         stl_p(header + 0x21c, initrd_size);
@@ -1063,15 +1069,32 @@ void x86_load_linux(X86MachineState *x86ms,
         load_image_size(dtb_filename, setup_data->data, dtb_size);
     }
 
-    memcpy(setup, header, MIN(sizeof(header), setup_size));
+    /*
+     * If we're starting an encrypted VM, it will be OVMF based, which uses the
+     * efi stub for booting and doesn't require any values to be placed in the
+     * kernel header.  We therefore don't update the header so the hash of the
+     * kernel on the other side of the fw_cfg interface matches the hash of the
+     * file the user passed in.
+     */
+    if (!sev_enabled()) {
+        memcpy(setup, header, MIN(sizeof(header), setup_size));
+    }
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
+    kernel_loader_context.kernel_data = (char *)kernel;
+    kernel_loader_context.kernel_size = kernel_size;
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
+    kernel_loader_context.setup_data = (char *)setup;
+    kernel_loader_context.setup_size = setup_size;
+
+    if (sev_enabled()) {
+        sev_add_kernel_loader_hashes(&kernel_loader_context, &error_fatal);
+    }
 
     option_rom[nb_option_roms].bootindex = 0;
     option_rom[nb_option_roms].name = "linuxboot.bin";
-- 
2.25.1

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Philippe Mathieu-Daudé]

Hi Dov,

Minor comments inlined.

On 6/21/21 9:05 PM, Dov Murik wrote:
> Add the sev_add_kernel_loader_hashes function to calculate the hashes of
> the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
> table area.  For this to work, OVMF must support an encrypted area to
> place the data which is advertised via a special GUID in the OVMF reset
> table.
> 
> The hashes of each of the files is calculated (or the string in the case
> of the cmdline with trailing '\0' included).  Each entry in the hashes
> table is GUID identified and since they're passed through the
> sev_encrypt_flash interface, the hashes will be accumulated by the PSP
> measurement (SEV_LAUNCH_MEASURE).
> 
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  target/i386/sev-stub.c |   5 ++
>  target/i386/sev.c      | 121 +++++++++++++++++++++++++++++++++++++++++
>  target/i386/sev_i386.h |  12 ++++
>  3 files changed, 138 insertions(+)
> 
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 0227cb5177..2b5e42d644 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -81,3 +81,8 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>      error_setg(errp, "SEV is not available in this QEMU");
>      return NULL;
>  }
> +
> +bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
> +{
> +    return false;

Can't happen, so:

      g_assert_not_reached();

> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 83df8c09f6..8e3f601bb6 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -23,6 +23,7 @@
>  #include "qemu/base64.h"
>  #include "qemu/module.h"
>  #include "qemu/uuid.h"
> +#include "crypto/hash.h"
>  #include "sysemu/kvm.h"
>  #include "sev_i386.h"
>  #include "sysemu/sysemu.h"
> @@ -83,6 +84,29 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
>      uint32_t reset_addr;
>  } SevInfoBlock;
>  
> +#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
> +typedef struct __attribute__((__packed__))

The codebase used to use QEMU_PACKED (see "qemu/compiler.h" but
apparently it isn't enforced.

 SevHashTableDescriptor {
> +    /* SEV hash table area guest address */
> +    uint32_t base;
> +    /* SEV hash table area size (in bytes) */
> +    uint32_t size;
> +} SevHashTableDescriptor;
> +
> +/* hard code sha256 digest size */
> +#define HASH_SIZE 32
> +
> +typedef struct __attribute__((__packed__)) SevHashTableEntry {
> +    uint8_t guid[16];

What about using QemuUUID?

> +    uint16_t len;
> +    uint8_t hash[HASH_SIZE];
> +} SevHashTableEntry;
> +
> +typedef struct __attribute__((__packed__)) SevHashTable {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    SevHashTableEntry entries[];
> +} SevHashTable;
> +
>  static SevGuestState *sev_guest;
>  static Error *sev_mig_blocker;
>  
> @@ -1077,6 +1101,103 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
>      return 0;
>  }
>  
> +static const uint8_t sev_hash_table_header_guid[] =
> +        UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
> +                0xd4, 0x11, 0xfd, 0x21);

Personally I'd have used:

static const QemuUUID sev_hash_table_header_guid = {
    .data = UUID_LE(...);
};

and added qemu_uuid_copy() to complete the API, but that's fine.

> +
> +static const uint8_t sev_kernel_entry_guid[] =
> +        UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
> +                0x72, 0xd2, 0x04, 0x5b);
> +static const uint8_t sev_initrd_entry_guid[] =
> +        UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
> +                0x91, 0x69, 0x78, 0x1d);
> +static const uint8_t sev_cmdline_entry_guid[] =
> +        UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
> +                0x4d, 0x36, 0xab, 0x2a);
> +
> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t *guid,
> +                                      const uint8_t *hash, size_t hash_len)
> +{
> +    memcpy(e->guid, guid, sizeof(e->guid));
> +    e->len = sizeof(*e);
> +    memcpy(e->hash, hash, hash_len);
> +}
> +
> +/*
> + * Add the hashes of the linux kernel/initrd/cmdline to an encrypted guest page
> + * which is included in SEV's initial memory measurement.
> + */
> +bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
> +{
> +    uint8_t *data;
> +    SevHashTableDescriptor *area;
> +    SevHashTable *ht;
> +    SevHashTableEntry *e;
> +    uint8_t hash_buf[HASH_SIZE];
> +    uint8_t *hash = hash_buf;
> +    size_t hash_len = sizeof(hash_buf);
> +    int ht_index = 0;
> +    int aligned_len;
> +
> +    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {

If we never use the data_len argument, can we simplify the prototype?

> +        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
> +        return false;
> +    }
> +    area = (SevHashTableDescriptor *)data;
> +
> +    ht = qemu_map_ram_ptr(NULL, area->base);
> +
> +    /* Populate the hashes table header */
> +    memcpy(ht->guid, sev_hash_table_header_guid, sizeof(ht->guid));
> +    ht->len = sizeof(*ht);
> +
> +    /* Calculate hash of kernel command-line */
> +    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
> +                           ctx->cmdline_size,
> +                           &hash, &hash_len, errp) < 0) {
> +        return false;
> +    }

Maybe move the qcrypto_hash_bytes() call before filling ht?

> +    e = &ht->entries[ht_index++];
> +    fill_sev_hash_table_entry(e, sev_cmdline_entry_guid, hash, hash_len);
> +
> +    /* Calculate hash of initrd */
> +    if (ctx->initrd_data) {
> +        if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
> +                               ctx->initrd_size, &hash, &hash_len, errp) < 0) {
> +            return false;
> +        }

Ah, now I see the pattern. Hmm OK then.

> +        e = &ht->entries[ht_index++];
> +        fill_sev_hash_table_entry(e, sev_initrd_entry_guid, hash, hash_len);
> +    }
> +
> +    /* Calculate hash of the kernel */
> +    struct iovec iov[2] = {
> +        { .iov_base = ctx->setup_data, .iov_len = ctx->setup_size },
> +        { .iov_base = ctx->kernel_data, .iov_len = ctx->kernel_size }
> +    };
> +    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, 2,
> +                            &hash, &hash_len, errp) < 0) {
> +        return false;
> +    }
> +    e = &ht->entries[ht_index++];
> +    fill_sev_hash_table_entry(e, sev_kernel_entry_guid, hash, hash_len);
> +
> +    /* now we have all the possible entries, finalize the hashes table */
> +    ht->len += ht_index * sizeof(*e);
> +    /* SEV len has to be 16 byte aligned */
> +    aligned_len = ROUND_UP(ht->len, 16);
> +    if (aligned_len != ht->len) {
> +        /* zero the excess data so the measurement can be reliably calculated */
> +        memset(&ht->entries[ht_index], 0, aligned_len - ht->len);
> +    }
> +
> +    if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
> +        return false;
> +    }
> +
> +    return true;
> +}

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

I found an issue with this patch when no -initrd is passed, see below.



On 21/06/2021 22:05, Dov Murik wrote:
> Add the sev_add_kernel_loader_hashes function to calculate the hashes of
> the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
> table area.  For this to work, OVMF must support an encrypted area to
> place the data which is advertised via a special GUID in the OVMF reset
> table.
> 
> The hashes of each of the files is calculated (or the string in the case
> of the cmdline with trailing '\0' included).  Each entry in the hashes
> table is GUID identified and since they're passed through the
> sev_encrypt_flash interface, the hashes will be accumulated by the PSP
> measurement (SEV_LAUNCH_MEASURE).
> 
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  target/i386/sev-stub.c |   5 ++
>  target/i386/sev.c      | 121 +++++++++++++++++++++++++++++++++++++++++
>  target/i386/sev_i386.h |  12 ++++
>  3 files changed, 138 insertions(+)
> 
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 0227cb5177..2b5e42d644 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -81,3 +81,8 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>      error_setg(errp, "SEV is not available in this QEMU");
>      return NULL;
>  }
> +
> +bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
> +{
> +    return false;
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 83df8c09f6..8e3f601bb6 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -23,6 +23,7 @@
>  #include "qemu/base64.h"
>  #include "qemu/module.h"
>  #include "qemu/uuid.h"
> +#include "crypto/hash.h"
>  #include "sysemu/kvm.h"
>  #include "sev_i386.h"
>  #include "sysemu/sysemu.h"
> @@ -83,6 +84,29 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
>      uint32_t reset_addr;
>  } SevInfoBlock;
>  
> +#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
> +typedef struct __attribute__((__packed__)) SevHashTableDescriptor {
> +    /* SEV hash table area guest address */
> +    uint32_t base;
> +    /* SEV hash table area size (in bytes) */
> +    uint32_t size;
> +} SevHashTableDescriptor;
> +
> +/* hard code sha256 digest size */
> +#define HASH_SIZE 32
> +
> +typedef struct __attribute__((__packed__)) SevHashTableEntry {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    uint8_t hash[HASH_SIZE];
> +} SevHashTableEntry;
> +
> +typedef struct __attribute__((__packed__)) SevHashTable {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    SevHashTableEntry entries[];
> +} SevHashTable;
> +
>  static SevGuestState *sev_guest;
>  static Error *sev_mig_blocker;
>  
> @@ -1077,6 +1101,103 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
>      return 0;
>  }
>  
> +static const uint8_t sev_hash_table_header_guid[] =
> +        UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
> +                0xd4, 0x11, 0xfd, 0x21);
> +
> +static const uint8_t sev_kernel_entry_guid[] =
> +        UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
> +                0x72, 0xd2, 0x04, 0x5b);
> +static const uint8_t sev_initrd_entry_guid[] =
> +        UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
> +                0x91, 0x69, 0x78, 0x1d);
> +static const uint8_t sev_cmdline_entry_guid[] =
> +        UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
> +                0x4d, 0x36, 0xab, 0x2a);
> +
> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t *guid,
> +                                      const uint8_t *hash, size_t hash_len)
> +{
> +    memcpy(e->guid, guid, sizeof(e->guid));
> +    e->len = sizeof(*e);
> +    memcpy(e->hash, hash, hash_len);
> +}
> +
> +/*
> + * Add the hashes of the linux kernel/initrd/cmdline to an encrypted guest page
> + * which is included in SEV's initial memory measurement.
> + */
> +bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
> +{
> +    uint8_t *data;
> +    SevHashTableDescriptor *area;
> +    SevHashTable *ht;
> +    SevHashTableEntry *e;
> +    uint8_t hash_buf[HASH_SIZE];
> +    uint8_t *hash = hash_buf;
> +    size_t hash_len = sizeof(hash_buf);
> +    int ht_index = 0;
> +    int aligned_len;
> +
> +    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> +        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
> +        return false;
> +    }
> +    area = (SevHashTableDescriptor *)data;
> +
> +    ht = qemu_map_ram_ptr(NULL, area->base);
> +
> +    /* Populate the hashes table header */
> +    memcpy(ht->guid, sev_hash_table_header_guid, sizeof(ht->guid));
> +    ht->len = sizeof(*ht);
> +
> +    /* Calculate hash of kernel command-line */
> +    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
> +                           ctx->cmdline_size,
> +                           &hash, &hash_len, errp) < 0) {
> +        return false;
> +    }
> +    e = &ht->entries[ht_index++];
> +    fill_sev_hash_table_entry(e, sev_cmdline_entry_guid, hash, hash_len);
> +
> +    /* Calculate hash of initrd */
> +    if (ctx->initrd_data) {
> +        if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
> +                               ctx->initrd_size, &hash, &hash_len, errp) < 0) {
> +            return false;
> +        }
> +        e = &ht->entries[ht_index++];
> +        fill_sev_hash_table_entry(e, sev_initrd_entry_guid, hash, hash_len);
> +    }


As can be seen, we conditionally add the hash table entry for initrd,
depending on whether the user passed the -initrd switch to QEMU.
However, in the OVMF side, initrd hash is *always* verified; in this
case it will be verified against a the hash of an empty buffer (sha256
starts with e3b0c44298fc...).  Since QEMU (this patch) doesn't add that
entry in the hashes table, we get an an access denied error in OVMF.

I think OVMF is doing the correct thing: the Guest Owner wishes to have
no initrd, and OVMF should verify that the initrd it reads is indeed
empty.  Therefore the measured SEV hashes table should include an entry
for initrd with the sha256 of an empty buffer.

(James - do you agree?)


I think we should remove this condition, and always fill the three
entries in the hashes table: cmdline, initrd, and kernel.  Here's the
treatment for missing arguments:

* No -cmdline: use the hash of the 1-byte "\0" string (this is already
  the case, no code changes needed).
* No -initrd: use the hash of the empty buffer.
* No -kernel: this code will not be reached at all (this is already the
  case, no code changes needed).


This leads to another possible modification: instead of array of
SevHashTableEntry entries[] in SevHashTable, we can have:

typedef struct __attribute__((__packed__)) SevHashTable {
    uint8_t guid[16];
    uint16_t len;
    SevHashTableEntry cmdline;
    SevHashTableEntry initrd;
    SevHashTableEntry kernel;
    uint8_t zero_padding[];  /* padded to 16-byte boundary */
} SevHashTable;


This could clear up things here; keeping the GUID+length in each entry
leaves the future option for a more modular table.

It will also make the order of entries clear, which is required for
calculating the expected measurement on the Guest Owner side.

This also means that the total length of the entire table is known in
advance (168 bytes; aligned to 176 bytes with zero padding).  Not sure
if this helps.


-Dov


> +
> +    /* Calculate hash of the kernel */
> +    struct iovec iov[2] = {
> +        { .iov_base = ctx->setup_data, .iov_len = ctx->setup_size },
> +        { .iov_base = ctx->kernel_data, .iov_len = ctx->kernel_size }
> +    };
> +    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, 2,
> +                            &hash, &hash_len, errp) < 0) {
> +        return false;
> +    }
> +    e = &ht->entries[ht_index++];
> +    fill_sev_hash_table_entry(e, sev_kernel_entry_guid, hash, hash_len);
> +
> +    /* now we have all the possible entries, finalize the hashes table */
> +    ht->len += ht_index * sizeof(*e);
> +    /* SEV len has to be 16 byte aligned */
> +    aligned_len = ROUND_UP(ht->len, 16);
> +    if (aligned_len != ht->len) {
> +        /* zero the excess data so the measurement can be reliably calculated */
> +        memset(&ht->entries[ht_index], 0, aligned_len - ht->len);
> +    }
> +
> +    if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  static void
>  sev_register_types(void)
>  {
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index ae6d840478..deb3eec409 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -28,6 +28,17 @@
>  #define SEV_POLICY_DOMAIN       0x10
>  #define SEV_POLICY_SEV          0x20
>  
> +typedef struct KernelLoaderContext {
> +    char *setup_data;
> +    size_t setup_size;
> +    char *kernel_data;
> +    size_t kernel_size;
> +    char *initrd_data;
> +    size_t initrd_size;
> +    char *cmdline_data;
> +    size_t cmdline_size;
> +} KernelLoaderContext;
> +
>  extern bool sev_es_enabled(void);
>  extern uint64_t sev_get_me_mask(void);
>  extern SevInfo *sev_get_info(void);
> @@ -37,5 +48,6 @@ extern char *sev_get_launch_measurement(void);
>  extern SevCapability *sev_get_capabilities(Error **errp);
>  extern SevAttestationReport *
>  sev_get_attestation_report(const char *mnonce, Error **errp);
> +extern bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp);
>  
>  #endif
> 

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

Hi Philippe,

On 21/06/2021 23:32, Philippe Mathieu-Daudé wrote:
> Hi Dov,
> 
> Minor comments inlined.
> 
> On 6/21/21 9:05 PM, Dov Murik wrote:
>> Add the sev_add_kernel_loader_hashes function to calculate the hashes of
>> the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
>> table area.  For this to work, OVMF must support an encrypted area to
>> place the data which is advertised via a special GUID in the OVMF reset
>> table.
>>
>> The hashes of each of the files is calculated (or the string in the case
>> of the cmdline with trailing '\0' included).  Each entry in the hashes
>> table is GUID identified and since they're passed through the
>> sev_encrypt_flash interface, the hashes will be accumulated by the PSP
>> measurement (SEV_LAUNCH_MEASURE).
>>
>> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  target/i386/sev-stub.c |   5 ++
>>  target/i386/sev.c      | 121 +++++++++++++++++++++++++++++++++++++++++
>>  target/i386/sev_i386.h |  12 ++++
>>  3 files changed, 138 insertions(+)
>>
>> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
>> index 0227cb5177..2b5e42d644 100644
>> --- a/target/i386/sev-stub.c
>> +++ b/target/i386/sev-stub.c
>> @@ -81,3 +81,8 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>>      error_setg(errp, "SEV is not available in this QEMU");
>>      return NULL;
>>  }
>> +
>> +bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
>> +{
>> +    return false;
> 
> Can't happen, so:
> 
>       g_assert_not_reached();
> 

OK, I'll use it.

I guess the comment is relevant to other functions in that file as well
(e.g., sev_encrypt_flash), but I'll leave that to your SEV-housekeeping
series.


>> +}
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 83df8c09f6..8e3f601bb6 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -23,6 +23,7 @@
>>  #include "qemu/base64.h"
>>  #include "qemu/module.h"
>>  #include "qemu/uuid.h"
>> +#include "crypto/hash.h"
>>  #include "sysemu/kvm.h"
>>  #include "sev_i386.h"
>>  #include "sysemu/sysemu.h"
>> @@ -83,6 +84,29 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
>>      uint32_t reset_addr;
>>  } SevInfoBlock;
>>  
>> +#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
>> +typedef struct __attribute__((__packed__))
> 
> The codebase used to use QEMU_PACKED (see "qemu/compiler.h" but
> apparently it isn't enforced.
> 

I can use it.


>  SevHashTableDescriptor {
>> +    /* SEV hash table area guest address */
>> +    uint32_t base;
>> +    /* SEV hash table area size (in bytes) */
>> +    uint32_t size;
>> +} SevHashTableDescriptor;
>> +
>> +/* hard code sha256 digest size */
>> +#define HASH_SIZE 32
>> +
>> +typedef struct __attribute__((__packed__)) SevHashTableEntry {
>> +    uint8_t guid[16];
> 
> What about using QemuUUID?
> 

I agree. I'll use it, coupled with your .data init below.


>> +    uint16_t len;
>> +    uint8_t hash[HASH_SIZE];
>> +} SevHashTableEntry;
>> +
>> +typedef struct __attribute__((__packed__)) SevHashTable {
>> +    uint8_t guid[16];
>> +    uint16_t len;
>> +    SevHashTableEntry entries[];
>> +} SevHashTable;
>> +
>>  static SevGuestState *sev_guest;
>>  static Error *sev_mig_blocker;
>>  
>> @@ -1077,6 +1101,103 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
>>      return 0;
>>  }
>>  
>> +static const uint8_t sev_hash_table_header_guid[] =
>> +        UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
>> +                0xd4, 0x11, 0xfd, 0x21);
> 
> Personally I'd have used:
> 
> static const QemuUUID sev_hash_table_header_guid = {
>     .data = UUID_LE(...);
> };

Yes, I'll use this.

> 
> and added qemu_uuid_copy() to complete the API, but that's fine.

I think simple C assignment works for structs (and hence QemuUUID),
something like:

    SevHashTable *ht = ...;
    ht->guid = sev_hash_table_header_guid;

(where both ht->guid and sev_hash_table_header_guid are QemuUUID.)


> 
>> +
>> +static const uint8_t sev_kernel_entry_guid[] =
>> +        UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
>> +                0x72, 0xd2, 0x04, 0x5b);
>> +static const uint8_t sev_initrd_entry_guid[] =
>> +        UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
>> +                0x91, 0x69, 0x78, 0x1d);
>> +static const uint8_t sev_cmdline_entry_guid[] =
>> +        UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
>> +                0x4d, 0x36, 0xab, 0x2a);
>> +
>> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t *guid,
>> +                                      const uint8_t *hash, size_t hash_len)
>> +{
>> +    memcpy(e->guid, guid, sizeof(e->guid));
>> +    e->len = sizeof(*e);
>> +    memcpy(e->hash, hash, hash_len);
>> +}
>> +
>> +/*
>> + * Add the hashes of the linux kernel/initrd/cmdline to an encrypted guest page
>> + * which is included in SEV's initial memory measurement.
>> + */
>> +bool sev_add_kernel_loader_hashes(KernelLoaderContext *ctx, Error **errp)
>> +{
>> +    uint8_t *data;
>> +    SevHashTableDescriptor *area;
>> +    SevHashTable *ht;
>> +    SevHashTableEntry *e;
>> +    uint8_t hash_buf[HASH_SIZE];
>> +    uint8_t *hash = hash_buf;
>> +    size_t hash_len = sizeof(hash_buf);
>> +    int ht_index = 0;
>> +    int aligned_len;
>> +
>> +    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> 
> If we never use the data_len argument, can we simplify the prototype?

The current uses for the OVMF reset vector GUIDed table is for simple
structs with known length (secret injection page address, SEV-ES reset
address, SEV table of hashes address).  But keeping the length there
allows adding variable-sized entries such as strings/blobs.



> 
>> +        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
>> +        return false;
>> +    }
>> +    area = (SevHashTableDescriptor *)data;
>> +
>> +    ht = qemu_map_ram_ptr(NULL, area->base);
>> +
>> +    /* Populate the hashes table header */
>> +    memcpy(ht->guid, sev_hash_table_header_guid, sizeof(ht->guid));
>> +    ht->len = sizeof(*ht);
>> +
>> +    /* Calculate hash of kernel command-line */
>> +    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
>> +                           ctx->cmdline_size,
>> +                           &hash, &hash_len, errp) < 0) {
>> +        return false;
>> +    }
> 
> Maybe move the qcrypto_hash_bytes() call before filling ht?

(below)

> 
>> +    e = &ht->entries[ht_index++];
>> +    fill_sev_hash_table_entry(e, sev_cmdline_entry_guid, hash, hash_len);
>> +
>> +    /* Calculate hash of initrd */
>> +    if (ctx->initrd_data) {
>> +        if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
>> +                               ctx->initrd_size, &hash, &hash_len, errp) < 0) {
>> +            return false;
>> +        }
> 
> Ah, now I see the pattern. Hmm OK then.
> 

But this might change if initrd_hash is no longer optional (see separate
self-reply to this patch).  In such a case I'll probably first calculate
all the three hashes, and then fill in the SevHashTable struct.


-Dov


>> +        e = &ht->entries[ht_index++];
>> +        fill_sev_hash_table_entry(e, sev_initrd_entry_guid, hash, hash_len);
>> +    }
>> +
>> +    /* Calculate hash of the kernel */
>> +    struct iovec iov[2] = {
>> +        { .iov_base = ctx->setup_data, .iov_len = ctx->setup_size },
>> +        { .iov_base = ctx->kernel_data, .iov_len = ctx->kernel_size }
>> +    };
>> +    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, 2,
>> +                            &hash, &hash_len, errp) < 0) {
>> +        return false;
>> +    }
>> +    e = &ht->entries[ht_index++];
>> +    fill_sev_hash_table_entry(e, sev_kernel_entry_guid, hash, hash_len);
>> +
>> +    /* now we have all the possible entries, finalize the hashes table */
>> +    ht->len += ht_index * sizeof(*e);
>> +    /* SEV len has to be 16 byte aligned */
>> +    aligned_len = ROUND_UP(ht->len, 16);
>> +    if (aligned_len != ht->len) {
>> +        /* zero the excess data so the measurement can be reliably calculated */
>> +        memset(&ht->entries[ht_index], 0, aligned_len - ht->len);
>> +    }
>> +
>> +    if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
> 

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Philippe Mathieu-Daudé]

On 6/22/21 11:44 AM, Dov Murik wrote:
> On 21/06/2021 23:32, Philippe Mathieu-Daudé wrote:

>> and added qemu_uuid_copy() to complete the API, but that's fine.
> 
> I think simple C assignment works for structs (and hence QemuUUID),
> something like:
> 
>     SevHashTable *ht = ...;
>     ht->guid = sev_hash_table_header_guid;
> 
> (where both ht->guid and sev_hash_table_header_guid are QemuUUID.)

OK.

>>> +    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>>
>> If we never use the data_len argument, can we simplify the prototype?
> 
> The current uses for the OVMF reset vector GUIDed table is for simple
> structs with known length (secret injection page address, SEV-ES reset
> address, SEV table of hashes address).  But keeping the length there
> allows adding variable-sized entries such as strings/blobs.

OK. Good opportunity to document the prototype declaration ;)

> 
>>
>>> +        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
>>> +        return false;
>>> +    }
>>> +    area = (SevHashTableDescriptor *)data;
>>> +
>>> +    ht = qemu_map_ram_ptr(NULL, area->base);
>>> +
>>> +    /* Populate the hashes table header */
>>> +    memcpy(ht->guid, sev_hash_table_header_guid, sizeof(ht->guid));
>>> +    ht->len = sizeof(*ht);
>>> +
>>> +    /* Calculate hash of kernel command-line */
>>> +    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
>>> +                           ctx->cmdline_size,
>>> +                           &hash, &hash_len, errp) < 0) {
>>> +        return false;
>>> +    }
>>
>> Maybe move the qcrypto_hash_bytes() call before filling ht?
> 
> (below)
> 
>>
>>> +    e = &ht->entries[ht_index++];
>>> +    fill_sev_hash_table_entry(e, sev_cmdline_entry_guid, hash, hash_len);
>>> +
>>> +    /* Calculate hash of initrd */
>>> +    if (ctx->initrd_data) {
>>> +        if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
>>> +                               ctx->initrd_size, &hash, &hash_len, errp) < 0) {
>>> +            return false;
>>> +        }
>>
>> Ah, now I see the pattern. Hmm OK then.
>>
> 
> But this might change if initrd_hash is no longer optional (see separate
> self-reply to this patch).  In such a case I'll probably first calculate
> all the three hashes, and then fill in the SevHashTable struct.

Yes, sounds simpler.

Regards,

Phil.

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

On 22/06/2021 12:49, Philippe Mathieu-Daudé wrote:
> On 6/22/21 11:44 AM, Dov Murik wrote:
>> On 21/06/2021 23:32, Philippe Mathieu-Daudé wrote:
> 

...

> 
>>>> +    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>>>
>>> If we never use the data_len argument, can we simplify the prototype?
>>
>> The current uses for the OVMF reset vector GUIDed table is for simple
>> structs with known length (secret injection page address, SEV-ES reset
>> address, SEV table of hashes address).  But keeping the length there
>> allows adding variable-sized entries such as strings/blobs.
> 
> OK. Good opportunity to document the prototype declaration ;)

Yep. I'll send as a separate standalone patch.


P.S.
In a previous version you mentioned you ran into issues with a qemu
build with SEV disabled.  I tried that by modifying
default-configs/devices/i386-softmmu.mak and uncommenting CONFIG_SEV=n
there.  Is there a friendlier way to create such a build?

I'm currently building with:

    cd build
    ../configure --target-list=x86_64-softmmu
    make


Thanks,
-Dov

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Philippe Mathieu-Daudé]

On 6/22/21 12:26 PM, Dov Murik wrote:
> 
> 
> On 22/06/2021 12:49, Philippe Mathieu-Daudé wrote:
>> On 6/22/21 11:44 AM, Dov Murik wrote:
>>> On 21/06/2021 23:32, Philippe Mathieu-Daudé wrote:
>>
> 
> ...
> 
>>
>>>>> +    if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>>>>
>>>> If we never use the data_len argument, can we simplify the prototype?
>>>
>>> The current uses for the OVMF reset vector GUIDed table is for simple
>>> structs with known length (secret injection page address, SEV-ES reset
>>> address, SEV table of hashes address).  But keeping the length there
>>> allows adding variable-sized entries such as strings/blobs.
>>
>> OK. Good opportunity to document the prototype declaration ;)
> 
> Yep. I'll send as a separate standalone patch.

Sure.

> P.S.
> In a previous version you mentioned you ran into issues with a qemu
> build with SEV disabled.  I tried that by modifying
> default-configs/devices/i386-softmmu.mak and uncommenting CONFIG_SEV=n
> there.  Is there a friendlier way to create such a build?

Unfortunately not yet in mainstream (distributions tune their builds,
often disabling all recent features, and enable them on a case by case
basis once it is well tested).

Thankfully Alex posted a series to add the possibility:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg817710.html

> 
> I'm currently building with:
> 
>     cd build
>     ../configure --target-list=x86_64-softmmu
>     make
> 
> 
> Thanks,
> -Dov
> 

Re: [PATCH v2 2/2] x86/sev: generate SEV kernel loader hashes in x86_load_linux

[Connor Kuehl]

On 6/21/21 2:05 PM, Dov Murik wrote:
> If SEV is enabled and a kernel is passed via -kernel, pass the hashes of
> kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV
> measured boot.
> 
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  hw/i386/x86.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index ed796fe6ba..5c46463d9f 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -45,6 +45,7 @@
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/intc/i8259.h"
>  #include "hw/rtc/mc146818rtc.h"
> +#include "target/i386/sev_i386.h"
>  
>  #include "hw/acpi/cpu_hotplug.h"
>  #include "hw/irq.h"
> @@ -778,6 +779,7 @@ void x86_load_linux(X86MachineState *x86ms,
>      const char *initrd_filename = machine->initrd_filename;
>      const char *dtb_filename = machine->dtb;
>      const char *kernel_cmdline = machine->kernel_cmdline;
> +    KernelLoaderContext kernel_loader_context = {};
>  
>      /* Align to 16 bytes as a paranoia measure */
>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -924,6 +926,8 @@ void x86_load_linux(X86MachineState *x86ms,
>      fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
>      fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
> +    kernel_loader_context.cmdline_data = (char *)kernel_cmdline;
> +    kernel_loader_context.cmdline_size = strlen(kernel_cmdline) + 1;

I just wanted to check my understanding: I'm guessing you didn't set
`kernel_loader_context.cmdline_size` to `cmdline_size` (defined above)
so guest owners don't have to be aware of whatever alignment precaution
QEMU takes when producing their own measurement, right?

Otherwise:

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Connor Kuehl]

On 6/21/21 2:05 PM, Dov Murik wrote:
> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t *guid,
> +                                      const uint8_t *hash, size_t hash_len)
> +{
> +    memcpy(e->guid, guid, sizeof(e->guid));
> +    e->len = sizeof(*e);
> +    memcpy(e->hash, hash, hash_len);

Should this memcpy be constrained to MIN(sizeof(e->hash), hash_len)? Or
perhaps an assert statement since I see below that this function's
caller sets this to HASH_SIZE which is currently == sizeof(e->hash).

Actually, the assert statement would be easier to debug if the input
to this function is ever unexpected, especially since it avoids an
outcome where the input is silently truncated; which is a pitfall that
that the memcpy clamping would fall into.

Connor

Re: [PATCH v2 2/2] x86/sev: generate SEV kernel loader hashes in x86_load_linux

[Dov Murik]

Hi Connor,

On 22/06/2021 23:55, Connor Kuehl wrote:
> On 6/21/21 2:05 PM, Dov Murik wrote:
>> If SEV is enabled and a kernel is passed via -kernel, pass the hashes of
>> kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV
>> measured boot.
>>
>> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  hw/i386/x86.c | 25 ++++++++++++++++++++++++-
>>  1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index ed796fe6ba..5c46463d9f 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -45,6 +45,7 @@
>>  #include "hw/i386/fw_cfg.h"
>>  #include "hw/intc/i8259.h"
>>  #include "hw/rtc/mc146818rtc.h"
>> +#include "target/i386/sev_i386.h"
>>  
>>  #include "hw/acpi/cpu_hotplug.h"
>>  #include "hw/irq.h"
>> @@ -778,6 +779,7 @@ void x86_load_linux(X86MachineState *x86ms,
>>      const char *initrd_filename = machine->initrd_filename;
>>      const char *dtb_filename = machine->dtb;
>>      const char *kernel_cmdline = machine->kernel_cmdline;
>> +    KernelLoaderContext kernel_loader_context = {};
>>  
>>      /* Align to 16 bytes as a paranoia measure */
>>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>> @@ -924,6 +926,8 @@ void x86_load_linux(X86MachineState *x86ms,
>>      fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr);
>>      fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
>>      fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
>> +    kernel_loader_context.cmdline_data = (char *)kernel_cmdline;
>> +    kernel_loader_context.cmdline_size = strlen(kernel_cmdline) + 1;
> 
> I just wanted to check my understanding: I'm guessing you didn't set
> `kernel_loader_context.cmdline_size` to `cmdline_size` (defined above)
> so guest owners don't have to be aware of whatever alignment precaution
> QEMU takes when producing their own measurement, right?

The hashes calculated by OVMF must be identical to the hashes calculated
by QEMU.  OVMF calculates a hash over the blob it receives from QEMU,
which is:

    fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
    fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);

That is -- the entire cmdline string including the terminating '\0'.

Here's the OVMF side that verifies the last byte is '\0':

    QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData);
    QemuFwCfgReadBytes (CommandLineSize, CommandLine);

    //
    // Verify NUL-termination of the command line.
    //
    if (CommandLine[CommandLineSize - 1] != '\0') {
      DEBUG ((DEBUG_ERROR, "%a: kernel command line is not NUL-terminated\n",
        __FUNCTION__));
      Status = EFI_PROTOCOL_ERROR;
      goto FreeCommandLine;
    }

(this is more or less where we're adding the hash verification in OVMF.)


As you said, the Guest Owner also calculates the hash in exact the same
way in order to verify the SEV PSP measurement.


As I understand from the x86_load_linux code, the rounded/aligned
`cmdline_size` is used for placing the command line in an aligned address 
inside the guest memory.  In the kernel I'm booting the `cmdline_addr` is
always 0x20000 and doesn't depend on cmdline_size at all.


> 
> Otherwise:
> 
> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 

Thanks!

-Dov

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

Hi Connor,

+cc: Daniel

On 23/06/2021 0:15, Connor Kuehl wrote:
> On 6/21/21 2:05 PM, Dov Murik wrote:
>> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t *guid,
>> +                                      const uint8_t *hash, size_t hash_len)
>> +{
>> +    memcpy(e->guid, guid, sizeof(e->guid));
>> +    e->len = sizeof(*e);
>> +    memcpy(e->hash, hash, hash_len);
> 
> Should this memcpy be constrained to MIN(sizeof(e->hash), hash_len)? Or
> perhaps an assert statement since I see below that this function's
> caller sets this to HASH_SIZE which is currently == sizeof(e->hash).
> 
> Actually, the assert statement would be easier to debug if the input
> to this function is ever unexpected, especially since it avoids an
> outcome where the input is silently truncated; which is a pitfall that
> that the memcpy clamping would fall into.

I agree.  I'll change it to:

    assert(hash_len == sizeof(e->hash));
    memcpy(e->hash, hash, sizeof(e->hash));

This way also the memcpy length is known at compile-time.



Related: I wondered if I could replace HASH_SIZE in:


  /* hard code sha256 digest size */
  #define HASH_SIZE 32

  typedef struct QEMU_PACKED SevHashTableEntry {
      QemuUUID guid;
      uint16_t len;
      uint8_t hash[HASH_SIZE];
  } SevHashTableEntry;


with some SHA256-related constant from crypto/hash.h, but I only found
the qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_SHA256) function which
doesn't work for setting sizes of arrays at compile-time.

Daniel: do you know what would be the proper way?


Thanks,
-Dov

Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Daniel P. Berrangé]

On Wed, Jun 23, 2021 at 11:41:56AM +0300, Dov Murik wrote:
> Hi Connor,
> 
> +cc: Daniel
> 
> On 23/06/2021 0:15, Connor Kuehl wrote:
> > On 6/21/21 2:05 PM, Dov Murik wrote:
> >> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t *guid,
> >> +                                      const uint8_t *hash, size_t hash_len)
> >> +{
> >> +    memcpy(e->guid, guid, sizeof(e->guid));
> >> +    e->len = sizeof(*e);
> >> +    memcpy(e->hash, hash, hash_len);
> > 
> > Should this memcpy be constrained to MIN(sizeof(e->hash), hash_len)? Or
> > perhaps an assert statement since I see below that this function's
> > caller sets this to HASH_SIZE which is currently == sizeof(e->hash).
> > 
> > Actually, the assert statement would be easier to debug if the input
> > to this function is ever unexpected, especially since it avoids an
> > outcome where the input is silently truncated; which is a pitfall that
> > that the memcpy clamping would fall into.
> 
> I agree.  I'll change it to:
> 
>     assert(hash_len == sizeof(e->hash));
>     memcpy(e->hash, hash, sizeof(e->hash));
> 
> This way also the memcpy length is known at compile-time.
> 
> 
> 
> Related: I wondered if I could replace HASH_SIZE in:
> 
> 
>   /* hard code sha256 digest size */
>   #define HASH_SIZE 32
> 
>   typedef struct QEMU_PACKED SevHashTableEntry {
>       QemuUUID guid;
>       uint16_t len;
>       uint8_t hash[HASH_SIZE];
>   } SevHashTableEntry;
> 
> 
> with some SHA256-related constant from crypto/hash.h, but I only found
> the qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_SHA256) function which
> doesn't work for setting sizes of arrays at compile-time.
> 
> Daniel: do you know what would be the proper way?

We don't have any public constants right now - they're just hardcoded
in hash.c struct. We could define public constants, and use those in
the struct instead, as well as in other callers.


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 v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

On 23/06/2021 11:49, Daniel P. Berrangé wrote:
> On Wed, Jun 23, 2021 at 11:41:56AM +0300, Dov Murik wrote:

...

>>
>> Related: I wondered if I could replace HASH_SIZE in:
>>
>>
>>   /* hard code sha256 digest size */
>>   #define HASH_SIZE 32
>>
>>   typedef struct QEMU_PACKED SevHashTableEntry {
>>       QemuUUID guid;
>>       uint16_t len;
>>       uint8_t hash[HASH_SIZE];
>>   } SevHashTableEntry;
>>
>>
>> with some SHA256-related constant from crypto/hash.h, but I only found
>> the qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_SHA256) function which
>> doesn't work for setting sizes of arrays at compile-time.
>>
>> Daniel: do you know what would be the proper way?
> 
> We don't have any public constants right now - they're just hardcoded
> in hash.c struct. We could define public constants, and use those in
> the struct instead, as well as in other callers.
> 

Thanks Daniel.

I see the exact same pattern in block/quorom.c (see HASH_LENGTH there).
I'll leave this change for a separate series.

-Dov