[PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline

Dov Murik posted 1 patch 2 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/next-importer-push tags/patchew/20210525065931.1628554-1-dovmurik@linux.ibm.com
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Eduardo Habkost <ehabkost@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 119 insertions(+), 1 deletion(-)
[PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Dov Murik 2 years, 10 months ago
From: James Bottomley <jejb@linux.ibm.com>

If the VM is using memory encryption and also specifies a kernel/initrd
or appended command line, calculate the hashes and add them to the
encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
in the kernel/initrd/cmdline via the fw_cfg interface).

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 memcrypt
interface, the hash of the encrypted data will be accumulated by the
PSP.

Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
[dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
strings, remove GCC pragma, fix checkpatch errors]
---

OVMF support for handling the table of hashes (verifying that the
kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
to the measured hashes in the table) will be posted soon to edk2-devel.

---
 hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 1 deletion(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index ed796fe6ba..d8e77b99b4 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -37,12 +37,16 @@
 #include "sysemu/replay.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpu-timers.h"
+#include "sysemu/sev.h"
+#include "exec/confidential-guest-support.h"
 #include "trace.h"
+#include "crypto/hash.h"
 
 #include "hw/i386/x86.h"
 #include "target/i386/cpu.h"
 #include "hw/i386/topology.h"
 #include "hw/i386/fw_cfg.h"
+#include "hw/i386/pc.h"
 #include "hw/intc/i8259.h"
 #include "hw/rtc/mc146818rtc.h"
 
@@ -758,6 +762,42 @@ static bool load_elfboot(const char *kernel_filename,
     return true;
 }
 
+struct sev_hash_table_descriptor {
+    uint32_t base;
+    uint32_t size;
+};
+
+/* hard code sha256 digest size */
+#define HASH_SIZE 32
+
+struct sev_hash_table_entry {
+    uint8_t guid[16];
+    uint16_t len;
+    uint8_t hash[HASH_SIZE];
+} __attribute__ ((packed));
+
+struct sev_hash_table {
+    uint8_t guid[16];
+    uint16_t len;
+    struct sev_hash_table_entry entries[];
+} __attribute__((packed));
+
+#define SEV_HASH_TABLE_RV_GUID "7255371f-3a3b-4b04-927b-1da6efa8d454"
+
+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);
+
 void x86_load_linux(X86MachineState *x86ms,
                     FWCfgState *fw_cfg,
                     int acpi_data_size,
@@ -778,6 +818,11 @@ 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;
+    uint8_t buf[HASH_SIZE];
+    uint8_t *hash = buf;
+    size_t hash_len = sizeof(buf);
+    struct sev_hash_table *sev_ht = NULL;
+    int sev_ht_index = 0;
 
     /* Align to 16 bytes as a paranoia measure */
     cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
         exit(1);
     }
 
+    if (machine->cgs && machine->cgs->ready) {
+        uint8_t *data;
+        struct sev_hash_table_descriptor *area;
+
+        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
+            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
+                    "no hash table guid\n");
+            exit(1);
+        }
+        area = (struct sev_hash_table_descriptor *)data;
+
+        sev_ht = qemu_map_ram_ptr(NULL, area->base);
+        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
+        sev_ht->len = sizeof(*sev_ht);
+    }
+
     /* kernel protocol version */
     if (ldl_p(header + 0x202) == 0x53726448) {
         protocol = lduw_p(header + 0x206);
@@ -925,6 +986,17 @@ void x86_load_linux(X86MachineState *x86ms,
     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);
 
+    if (sev_ht) {
+        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
+
+        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
+                           strlen(kernel_cmdline) + 1,
+                           &hash, &hash_len, &error_fatal);
+        memcpy(e->hash, hash, hash_len);
+        e->len = sizeof(*e);
+        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
+    }
+
     if (protocol >= 0x202) {
         stl_p(header + 0x228, cmdline_addr);
     } else {
@@ -1008,6 +1080,17 @@ void x86_load_linux(X86MachineState *x86ms,
 
         stl_p(header + 0x218, initrd_addr);
         stl_p(header + 0x21c, initrd_size);
+
+        if (sev_ht) {
+            struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
+
+            qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)initrd_data,
+                               initrd_size, &hash, &hash_len, &error_fatal);
+            memcpy(e->hash, hash, hash_len);
+            e->len = sizeof(*e);
+            memcpy(e->guid, sev_initrd_entry_guid, sizeof(e->guid));
+        }
+
     }
 
     /* load kernel and setup */
@@ -1063,7 +1146,17 @@ 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 doing an encrypted VM (sev_ht will be set), 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_ht) {
+        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);
@@ -1073,6 +1166,31 @@ void x86_load_linux(X86MachineState *x86ms,
     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);
 
+    if (sev_ht) {
+        struct iovec iov[2] = {
+            {.iov_base = (char *)setup, .iov_len = setup_size },
+            {.iov_base = (char *)kernel, .iov_len = kernel_size }
+        };
+        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
+        int len;
+
+        qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, 2,
+                            &hash, &hash_len, &error_fatal);
+        memcpy(e->hash, hash, hash_len);
+        e->len = sizeof(*e);
+        memcpy(e->guid, sev_kernel_entry_guid, sizeof(e->guid));
+
+        /* now we have all the possible entries, finalize the hash table */
+        sev_ht->len += sev_ht_index * sizeof(*e);
+        /* SEV len has to be 16 byte aligned */
+        len = ROUND_UP(sev_ht->len, 16);
+        if (len != sev_ht->len) {
+            /* zero the excess data so hash can be reliably calculated */
+            memset(&sev_ht->entries[sev_ht_index], 0, len - sev_ht->len);
+        }
+
+        sev_encrypt_flash((uint8_t *)sev_ht, len, &error_fatal);
+    }
     option_rom[nb_option_roms].bootindex = 0;
     option_rom[nb_option_roms].name = "linuxboot.bin";
     if (linuxboot_dma_enabled && fw_cfg_dma_enabled(fw_cfg)) {
-- 
2.25.1


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Dov Murik 2 years, 10 months ago

On 25/05/2021 9:59, Dov Murik wrote:
> From: James Bottomley <jejb@linux.ibm.com>
> 
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).
> 
> 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 memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
> 
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
> 


OVMF support was submitted to edk2-devel (patch series "Measured SEV
boot with kernel/initrd/cmdline"), which starts here:

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

-Dov

Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Michael S. Tsirkin 2 years, 8 months ago
On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
> From: James Bottomley <jejb@linux.ibm.com>
> 
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).

Sorry about asking basic questions so late in the game.
I'm a bit curious why this feature makes sense. If someone can play
with a Linux kernel command line isn't it pretty much game over security
wise? What protections does Linux have against malicious actors
manipulating the command line?


> 
> 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 memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
> 
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
> 
> ---
>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index ed796fe6ba..d8e77b99b4 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -37,12 +37,16 @@
>  #include "sysemu/replay.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpu-timers.h"
> +#include "sysemu/sev.h"
> +#include "exec/confidential-guest-support.h"
>  #include "trace.h"
> +#include "crypto/hash.h"
>  
>  #include "hw/i386/x86.h"
>  #include "target/i386/cpu.h"
>  #include "hw/i386/topology.h"
>  #include "hw/i386/fw_cfg.h"
> +#include "hw/i386/pc.h"
>  #include "hw/intc/i8259.h"
>  #include "hw/rtc/mc146818rtc.h"
>  
> @@ -758,6 +762,42 @@ static bool load_elfboot(const char *kernel_filename,
>      return true;
>  }
>  
> +struct sev_hash_table_descriptor {
> +    uint32_t base;
> +    uint32_t size;
> +};
> +
> +/* hard code sha256 digest size */
> +#define HASH_SIZE 32
> +
> +struct sev_hash_table_entry {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    uint8_t hash[HASH_SIZE];
> +} __attribute__ ((packed));
> +
> +struct sev_hash_table {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    struct sev_hash_table_entry entries[];
> +} __attribute__((packed));
> +
> +#define SEV_HASH_TABLE_RV_GUID "7255371f-3a3b-4b04-927b-1da6efa8d454"
> +
> +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);
> +
>  void x86_load_linux(X86MachineState *x86ms,
>                      FWCfgState *fw_cfg,
>                      int acpi_data_size,
> @@ -778,6 +818,11 @@ 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;
> +    uint8_t buf[HASH_SIZE];
> +    uint8_t *hash = buf;
> +    size_t hash_len = sizeof(buf);
> +    struct sev_hash_table *sev_ht = NULL;
> +    int sev_ht_index = 0;
>  
>      /* Align to 16 bytes as a paranoia measure */
>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>          exit(1);
>      }
>  
> +    if (machine->cgs && machine->cgs->ready) {
> +        uint8_t *data;
> +        struct sev_hash_table_descriptor *area;
> +
> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
> +                    "no hash table guid\n");
> +            exit(1);
> +        }
> +        area = (struct sev_hash_table_descriptor *)data;
> +
> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
> +        sev_ht->len = sizeof(*sev_ht);
> +    }
> +
>      /* kernel protocol version */
>      if (ldl_p(header + 0x202) == 0x53726448) {
>          protocol = lduw_p(header + 0x206);
> @@ -925,6 +986,17 @@ void x86_load_linux(X86MachineState *x86ms,
>      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);
>  
> +    if (sev_ht) {
> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +
> +        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> +                           strlen(kernel_cmdline) + 1,
> +                           &hash, &hash_len, &error_fatal);
> +        memcpy(e->hash, hash, hash_len);
> +        e->len = sizeof(*e);
> +        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> +    }
> +
>      if (protocol >= 0x202) {
>          stl_p(header + 0x228, cmdline_addr);
>      } else {
> @@ -1008,6 +1080,17 @@ void x86_load_linux(X86MachineState *x86ms,
>  
>          stl_p(header + 0x218, initrd_addr);
>          stl_p(header + 0x21c, initrd_size);
> +
> +        if (sev_ht) {
> +            struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +
> +            qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)initrd_data,
> +                               initrd_size, &hash, &hash_len, &error_fatal);
> +            memcpy(e->hash, hash, hash_len);
> +            e->len = sizeof(*e);
> +            memcpy(e->guid, sev_initrd_entry_guid, sizeof(e->guid));
> +        }
> +
>      }
>  
>      /* load kernel and setup */
> @@ -1063,7 +1146,17 @@ 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 doing an encrypted VM (sev_ht will be set), 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_ht) {
> +        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);
> @@ -1073,6 +1166,31 @@ void x86_load_linux(X86MachineState *x86ms,
>      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);
>  
> +    if (sev_ht) {
> +        struct iovec iov[2] = {
> +            {.iov_base = (char *)setup, .iov_len = setup_size },
> +            {.iov_base = (char *)kernel, .iov_len = kernel_size }
> +        };
> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +        int len;
> +
> +        qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, 2,
> +                            &hash, &hash_len, &error_fatal);
> +        memcpy(e->hash, hash, hash_len);
> +        e->len = sizeof(*e);
> +        memcpy(e->guid, sev_kernel_entry_guid, sizeof(e->guid));
> +
> +        /* now we have all the possible entries, finalize the hash table */
> +        sev_ht->len += sev_ht_index * sizeof(*e);
> +        /* SEV len has to be 16 byte aligned */
> +        len = ROUND_UP(sev_ht->len, 16);
> +        if (len != sev_ht->len) {
> +            /* zero the excess data so hash can be reliably calculated */
> +            memset(&sev_ht->entries[sev_ht_index], 0, len - sev_ht->len);
> +        }
> +
> +        sev_encrypt_flash((uint8_t *)sev_ht, len, &error_fatal);
> +    }
>      option_rom[nb_option_roms].bootindex = 0;
>      option_rom[nb_option_roms].name = "linuxboot.bin";
>      if (linuxboot_dma_enabled && fw_cfg_dma_enabled(fw_cfg)) {
> -- 
> 2.25.1


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Dov Murik 2 years, 8 months ago
Hi Michael,

[+cc Connor, Dave]

On 03/07/2021 19:42, Michael S. Tsirkin wrote:
> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
>> From: James Bottomley <jejb@linux.ibm.com>
>>
>> If the VM is using memory encryption and also specifies a kernel/initrd
>> or appended command line, calculate the hashes and add them to the
>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
>> in the kernel/initrd/cmdline via the fw_cfg interface).
> 
> Sorry about asking basic questions so late in the game.

No worries. Please noice there's a newer version:

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


> I'm a bit curious why this feature makes sense. If someone can play
> with a Linux kernel command line isn't it pretty much game over security
> wise? What protections does Linux have against malicious actors
> manipulating the command line?
> 

You're right -- if the host can modify the kernel command-line it's a game over.

This is why this patch (together with the corresponding OVMF patches; still
under review) measures and verifies the content of the kernel blob and
the initrd blob *and* the command-line blob.

Any modification/omission of any of them by the host will make the expected
SEV PSP measurement invalid, which should then indicate to the Guest Owner that
something is wrong with this guest.  At that point the Guest Owner should
refuse to inject secrets into the guest (and also complain to the Cloud
Service Provider).

-Dov


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Michael S. Tsirkin 2 years, 8 months ago
On Sun, Jul 04, 2021 at 09:16:59AM +0300, Dov Murik wrote:
> Hi Michael,
> 
> [+cc Connor, Dave]
> 
> On 03/07/2021 19:42, Michael S. Tsirkin wrote:
> > On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
> >> From: James Bottomley <jejb@linux.ibm.com>
> >>
> >> If the VM is using memory encryption and also specifies a kernel/initrd
> >> or appended command line, calculate the hashes and add them to the
> >> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
> >> in the kernel/initrd/cmdline via the fw_cfg interface).
> > 
> > Sorry about asking basic questions so late in the game.
> 
> No worries. Please noice there's a newer version:
> 
> https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmurik@linux.ibm.com/
> 
> 
> > I'm a bit curious why this feature makes sense. If someone can play
> > with a Linux kernel command line isn't it pretty much game over security
> > wise? What protections does Linux have against malicious actors
> > manipulating the command line?
> > 
> 
> You're right -- if the host can modify the kernel command-line it's a game over.
> 
> This is why this patch (together with the corresponding OVMF patches; still
> under review) measures and verifies the content of the kernel blob and
> the initrd blob *and* the command-line blob.
> 
> Any modification/omission of any of them by the host will make the expected
> SEV PSP measurement invalid, which should then indicate to the Guest Owner that
> something is wrong with this guest.  At that point the Guest Owner should
> refuse to inject secrets into the guest (and also complain to the Cloud
> Service Provider).
> 
> -Dov

Got it, thanks!

-- 
MST


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Dov Murik 2 years, 9 months ago
ping


Reminder: this is to support secure (measured) boot with AMD SEV with
QEMU's -kernel/-initrd/-append switches.

The OVMF side of the implementation is under review (with some changes
requested), but so far no functional changes are exepcted from the QEMU
side, on top of this proposed patch.


(+Cc: Dave)

-Dov


On 25/05/2021 9:59, Dov Murik wrote:
> From: James Bottomley <jejb@linux.ibm.com>
> 
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).
> 
> 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 memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
> 
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
> 
> ---
>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index ed796fe6ba..d8e77b99b4 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -37,12 +37,16 @@
>  #include "sysemu/replay.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpu-timers.h"
> +#include "sysemu/sev.h"
> +#include "exec/confidential-guest-support.h"
>  #include "trace.h"
> +#include "crypto/hash.h"
>  
>  #include "hw/i386/x86.h"
>  #include "target/i386/cpu.h"
>  #include "hw/i386/topology.h"
>  #include "hw/i386/fw_cfg.h"
> +#include "hw/i386/pc.h"
>  #include "hw/intc/i8259.h"
>  #include "hw/rtc/mc146818rtc.h"
>  
> @@ -758,6 +762,42 @@ static bool load_elfboot(const char *kernel_filename,
>      return true;
>  }
>  
> +struct sev_hash_table_descriptor {
> +    uint32_t base;
> +    uint32_t size;
> +};
> +
> +/* hard code sha256 digest size */
> +#define HASH_SIZE 32
> +
> +struct sev_hash_table_entry {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    uint8_t hash[HASH_SIZE];
> +} __attribute__ ((packed));
> +
> +struct sev_hash_table {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    struct sev_hash_table_entry entries[];
> +} __attribute__((packed));
> +
> +#define SEV_HASH_TABLE_RV_GUID "7255371f-3a3b-4b04-927b-1da6efa8d454"
> +
> +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);
> +
>  void x86_load_linux(X86MachineState *x86ms,
>                      FWCfgState *fw_cfg,
>                      int acpi_data_size,
> @@ -778,6 +818,11 @@ 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;
> +    uint8_t buf[HASH_SIZE];
> +    uint8_t *hash = buf;
> +    size_t hash_len = sizeof(buf);
> +    struct sev_hash_table *sev_ht = NULL;
> +    int sev_ht_index = 0;
>  
>      /* Align to 16 bytes as a paranoia measure */
>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>          exit(1);
>      }
>  
> +    if (machine->cgs && machine->cgs->ready) {
> +        uint8_t *data;
> +        struct sev_hash_table_descriptor *area;
> +
> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
> +                    "no hash table guid\n");
> +            exit(1);
> +        }
> +        area = (struct sev_hash_table_descriptor *)data;
> +
> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
> +        sev_ht->len = sizeof(*sev_ht);
> +    }
> +
>      /* kernel protocol version */
>      if (ldl_p(header + 0x202) == 0x53726448) {
>          protocol = lduw_p(header + 0x206);
> @@ -925,6 +986,17 @@ void x86_load_linux(X86MachineState *x86ms,
>      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);
>  
> +    if (sev_ht) {
> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +
> +        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> +                           strlen(kernel_cmdline) + 1,
> +                           &hash, &hash_len, &error_fatal);
> +        memcpy(e->hash, hash, hash_len);
> +        e->len = sizeof(*e);
> +        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> +    }
> +
>      if (protocol >= 0x202) {
>          stl_p(header + 0x228, cmdline_addr);
>      } else {
> @@ -1008,6 +1080,17 @@ void x86_load_linux(X86MachineState *x86ms,
>  
>          stl_p(header + 0x218, initrd_addr);
>          stl_p(header + 0x21c, initrd_size);
> +
> +        if (sev_ht) {
> +            struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +
> +            qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)initrd_data,
> +                               initrd_size, &hash, &hash_len, &error_fatal);
> +            memcpy(e->hash, hash, hash_len);
> +            e->len = sizeof(*e);
> +            memcpy(e->guid, sev_initrd_entry_guid, sizeof(e->guid));
> +        }
> +
>      }
>  
>      /* load kernel and setup */
> @@ -1063,7 +1146,17 @@ 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 doing an encrypted VM (sev_ht will be set), 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_ht) {
> +        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);
> @@ -1073,6 +1166,31 @@ void x86_load_linux(X86MachineState *x86ms,
>      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);
>  
> +    if (sev_ht) {
> +        struct iovec iov[2] = {
> +            {.iov_base = (char *)setup, .iov_len = setup_size },
> +            {.iov_base = (char *)kernel, .iov_len = kernel_size }
> +        };
> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +        int len;
> +
> +        qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, 2,
> +                            &hash, &hash_len, &error_fatal);
> +        memcpy(e->hash, hash, hash_len);
> +        e->len = sizeof(*e);
> +        memcpy(e->guid, sev_kernel_entry_guid, sizeof(e->guid));
> +
> +        /* now we have all the possible entries, finalize the hash table */
> +        sev_ht->len += sev_ht_index * sizeof(*e);
> +        /* SEV len has to be 16 byte aligned */
> +        len = ROUND_UP(sev_ht->len, 16);
> +        if (len != sev_ht->len) {
> +            /* zero the excess data so hash can be reliably calculated */
> +            memset(&sev_ht->entries[sev_ht_index], 0, len - sev_ht->len);
> +        }
> +
> +        sev_encrypt_flash((uint8_t *)sev_ht, len, &error_fatal);
> +    }
>      option_rom[nb_option_roms].bootindex = 0;
>      option_rom[nb_option_roms].name = "linuxboot.bin";
>      if (linuxboot_dma_enabled && fw_cfg_dma_enabled(fw_cfg)) {
> 

Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Eduardo Habkost 2 years, 9 months ago
On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
> From: James Bottomley <jejb@linux.ibm.com>
> 
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).
> 
> 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 memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
> 
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
> 
> ---
>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 1 deletion(-)
> 

This is not an objection to the patch itself, but: can we do
something to move all sev-related code to sev.c?  It would make
the process of assigning a maintainer and reviewing/merging
future patches much simpler.

I am not familiar with SEV internals, so my only question is
about configurations where SEV is disabled:

[...]
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> @@ -778,6 +818,11 @@ 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;
> +    uint8_t buf[HASH_SIZE];
> +    uint8_t *hash = buf;
> +    size_t hash_len = sizeof(buf);
> +    struct sev_hash_table *sev_ht = NULL;
> +    int sev_ht_index = 0;
>  
>      /* Align to 16 bytes as a paranoia measure */
>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>          exit(1);
>      }
>  
> +    if (machine->cgs && machine->cgs->ready) {

machine->cgs doesn't seem to be a SEV-specific field.
What if machine->cgs->ready is set but SEV is disabled?

> +        uint8_t *data;
> +        struct sev_hash_table_descriptor *area;
> +
> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
> +                    "no hash table guid\n");
> +            exit(1);
> +        }
> +        area = (struct sev_hash_table_descriptor *)data;
> +
> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
> +        sev_ht->len = sizeof(*sev_ht);
> +    }
> +
>      /* kernel protocol version */
>      if (ldl_p(header + 0x202) == 0x53726448) {
>          protocol = lduw_p(header + 0x206);
[...]

-- 
Eduardo


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Philippe Mathieu-Daudé 2 years, 9 months ago
Hi Dov, James,

+Connor who asked to be reviewer.

On 6/15/21 5:20 PM, Eduardo Habkost wrote:
> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
>> From: James Bottomley <jejb@linux.ibm.com>
>>
>> If the VM is using memory encryption and also specifies a kernel/initrd
>> or appended command line, calculate the hashes and add them to the
>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>
>> 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 memcrypt
>> interface, the hash of the encrypted data will be accumulated by the
>> PSP.
>>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
>> strings, remove GCC pragma, fix checkpatch errors]
>> ---
>>
>> OVMF support for handling the table of hashes (verifying that the
>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>
>> ---
>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 119 insertions(+), 1 deletion(-)
>>
> 
> This is not an objection to the patch itself, but: can we do
> something to move all sev-related code to sev.c?  It would make
> the process of assigning a maintainer and reviewing/merging
> future patches much simpler.
> 
> I am not familiar with SEV internals, so my only question is
> about configurations where SEV is disabled:
> 
> [...]
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> @@ -778,6 +818,11 @@ 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;
>> +    uint8_t buf[HASH_SIZE];
>> +    uint8_t *hash = buf;
>> +    size_t hash_len = sizeof(buf);
>> +    struct sev_hash_table *sev_ht = NULL;
>> +    int sev_ht_index = 0;

Can you move all these variable into a structure, and use it as a
SEV loader context?

Then each block of code you added can be moved to its own function,
self-described, working with the previous context.

The functions can be declared in sev_i386.h and defined in sev.c as
Eduardo suggested.

>>  
>>      /* Align to 16 bytes as a paranoia measure */
>>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>>          exit(1);
>>      }
>>  
>> +    if (machine->cgs && machine->cgs->ready) {
> 
> machine->cgs doesn't seem to be a SEV-specific field.
> What if machine->cgs->ready is set but SEV is disabled?
> 
>> +        uint8_t *data;
>> +        struct sev_hash_table_descriptor *area;
>> +
>> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
>> +                    "no hash table guid\n");
>> +            exit(1);
>> +        }
>> +        area = (struct sev_hash_table_descriptor *)data;
>> +
>> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
>> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
>> +        sev_ht->len = sizeof(*sev_ht);
>> +    }
>> +
>>      /* kernel protocol version */
>>      if (ldl_p(header + 0x202) == 0x53726448) {
>>          protocol = lduw_p(header + 0x206);
> [...]
> 


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Dov Murik 2 years, 9 months ago

On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
> Hi Dov, James,
> 
> +Connor who asked to be reviewer.
> 
> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
>> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
>>> From: James Bottomley <jejb@linux.ibm.com>
>>>
>>> If the VM is using memory encryption and also specifies a kernel/initrd
>>> or appended command line, calculate the hashes and add them to the
>>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
>>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>>
>>> 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 memcrypt
>>> interface, the hash of the encrypted data will be accumulated by the
>>> PSP.
>>>
>>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
>>> strings, remove GCC pragma, fix checkpatch errors]
>>> ---
>>>
>>> OVMF support for handling the table of hashes (verifying that the
>>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>>
>>> ---
>>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 119 insertions(+), 1 deletion(-)
>>>
>>
>> This is not an objection to the patch itself, but: can we do
>> something to move all sev-related code to sev.c?  It would make
>> the process of assigning a maintainer and reviewing/merging
>> future patches much simpler.
>>
>> I am not familiar with SEV internals, so my only question is
>> about configurations where SEV is disabled:
>>
>> [...]
>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>> @@ -778,6 +818,11 @@ 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;
>>> +    uint8_t buf[HASH_SIZE];
>>> +    uint8_t *hash = buf;
>>> +    size_t hash_len = sizeof(buf);
>>> +    struct sev_hash_table *sev_ht = NULL;
>>> +    int sev_ht_index = 0;
> 
> Can you move all these variable into a structure, and use it as a
> SEV loader context?
> 
> Then each block of code you added can be moved to its own function,
> self-described, working with the previous context.
> 
> The functions can be declared in sev_i386.h and defined in sev.c as
> Eduardo suggested.
> 

Thanks Philippe, I'll try this approach.


I assume you mean that an addition like this:

+    if (sev_ht) {
+        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
+
+        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
+                           strlen(kernel_cmdline) + 1,
+                           &hash, &hash_len, &error_fatal);
+        memcpy(e->hash, hash, hash_len);
+        e->len = sizeof(*e);
+        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
+    }

will be extracted to a function, and here (in x86_load_linux()) replaced
with a single call:

    sev_kernel_loader_calc_cmdline_hash(&sev_loader_context, kernel_cmdline);
  
and that function will have an empty stub in non-SEV builds, and a do-
nothing condition (at the beginning) if it's an SEV-disabled VM, and
will do the actual work in SEV VMs.

Right?


Also, should I base my next version on top of the current master, or on
top of your SEV-Housekeeping patch series, or on top of some other tree?


-Dov

>>>  
>>>      /* Align to 16 bytes as a paranoia measure */
>>>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>>>          exit(1);
>>>      }
>>>  
>>> +    if (machine->cgs && machine->cgs->ready) {
>>
>> machine->cgs doesn't seem to be a SEV-specific field.
>> What if machine->cgs->ready is set but SEV is disabled?
>>
>>> +        uint8_t *data;
>>> +        struct sev_hash_table_descriptor *area;
>>> +
>>> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>>> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
>>> +                    "no hash table guid\n");
>>> +            exit(1);
>>> +        }
>>> +        area = (struct sev_hash_table_descriptor *)data;
>>> +
>>> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
>>> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
>>> +        sev_ht->len = sizeof(*sev_ht);
>>> +    }
>>> +
>>>      /* kernel protocol version */
>>>      if (ldl_p(header + 0x202) == 0x53726448) {
>>>          protocol = lduw_p(header + 0x206);
>> [...]
>>
> 

Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Philippe Mathieu-Daudé 2 years, 9 months ago
Hi Dov,

+Thomas

On 6/17/21 2:48 PM, Dov Murik wrote:
> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
>> Hi Dov, James,
>>
>> +Connor who asked to be reviewer.
>>
>> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
>>> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
>>>> From: James Bottomley <jejb@linux.ibm.com>
>>>>
>>>> If the VM is using memory encryption and also specifies a kernel/initrd
>>>> or appended command line, calculate the hashes and add them to the
>>>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
>>>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>>>
>>>> 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 memcrypt
>>>> interface, the hash of the encrypted data will be accumulated by the
>>>> PSP.
>>>>
>>>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
>>>> strings, remove GCC pragma, fix checkpatch errors]
>>>> ---
>>>>
>>>> OVMF support for handling the table of hashes (verifying that the
>>>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>>>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>>>
>>>> ---
>>>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 119 insertions(+), 1 deletion(-)
>>>>
>>>
>>> This is not an objection to the patch itself, but: can we do
>>> something to move all sev-related code to sev.c?  It would make
>>> the process of assigning a maintainer and reviewing/merging
>>> future patches much simpler.
>>>
>>> I am not familiar with SEV internals, so my only question is
>>> about configurations where SEV is disabled:
>>>
>>> [...]
>>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>>> @@ -778,6 +818,11 @@ 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;
>>>> +    uint8_t buf[HASH_SIZE];
>>>> +    uint8_t *hash = buf;
>>>> +    size_t hash_len = sizeof(buf);
>>>> +    struct sev_hash_table *sev_ht = NULL;
>>>> +    int sev_ht_index = 0;
>>
>> Can you move all these variable into a structure, and use it as a
>> SEV loader context?
>>
>> Then each block of code you added can be moved to its own function,
>> self-described, working with the previous context.
>>
>> The functions can be declared in sev_i386.h and defined in sev.c as
>> Eduardo suggested.
>>
> 
> Thanks Philippe, I'll try this approach.
> 
> 
> I assume you mean that an addition like this:
> 
> +    if (sev_ht) {
> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +
> +        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> +                           strlen(kernel_cmdline) + 1,
> +                           &hash, &hash_len, &error_fatal);
> +        memcpy(e->hash, hash, hash_len);
> +        e->len = sizeof(*e);
> +        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> +    }
> 
> will be extracted to a function, and here (in x86_load_linux()) replaced
> with a single call:
> 
>     sev_kernel_loader_calc_cmdline_hash(&sev_loader_context, kernel_cmdline);
>   
> and that function will have an empty stub in non-SEV builds, and a do-
> nothing condition (at the beginning) if it's an SEV-disabled VM, and
> will do the actual work in SEV VMs.

This works, but I'd rather use:

  if (sev_enabled()) {
      sev_kernel_loader_calc_cmdline_hash(&sev_loader_context,
                                          kernel_cmdline);
  }

And have sev_enabled() defined as:

#ifdef CONFIG_SEV
bool sev_enabled(void);
#else
#define sev_enabled() false
#endif

So the compiler could elide the statement if SEV is disabled,
and stub is not necessary.

But that means we'd need to add "#include CONFIG_DEVICES" in
a sysemu/ header, which looks like an anti-pattern.

Thomas / Paolo, what do you think?

> Also, should I base my next version on top of the current master, or on
> top of your SEV-Housekeeping patch series, or on top of some other tree?

It depends its shape :> If the maintainers disagree with it, you better
base your patch on Eduardo's tree. Indeed the code will be different.
If you think the housekeeping is worthwhile, you could also review it
to increase the odds to get it queued ;)

Regards,

Phil.


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Thomas Huth 2 years, 9 months ago
On 17/06/2021 17.48, Philippe Mathieu-Daudé wrote:
[...]
> This works, but I'd rather use:
> 
>    if (sev_enabled()) {
>        sev_kernel_loader_calc_cmdline_hash(&sev_loader_context,
>                                            kernel_cmdline);
>    }
> 
> And have sev_enabled() defined as:
> 
> #ifdef CONFIG_SEV
> bool sev_enabled(void);
> #else
> #define sev_enabled() false
> #endif
> 
> So the compiler could elide the statement if SEV is disabled,
> and stub is not necessary.
> 
> But that means we'd need to add "#include CONFIG_DEVICES" in
> a sysemu/ header, which looks like an anti-pattern.
> 
> Thomas / Paolo, what do you think?

I'd only do that if you are very, very sure that the header file is  only 
included from target-specific files. Otherwise this will of course cause 
more trouble than benefit.

  Thomas


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Philippe Mathieu-Daudé 2 years, 9 months ago
On 6/21/21 10:44 AM, Thomas Huth wrote:
> On 17/06/2021 17.48, Philippe Mathieu-Daudé wrote:
> [...]
>> This works, but I'd rather use:
>>
>>    if (sev_enabled()) {
>>        sev_kernel_loader_calc_cmdline_hash(&sev_loader_context,
>>                                            kernel_cmdline);
>>    }
>>
>> And have sev_enabled() defined as:
>>
>> #ifdef CONFIG_SEV
>> bool sev_enabled(void);
>> #else
>> #define sev_enabled() false
>> #endif
>>
>> So the compiler could elide the statement if SEV is disabled,
>> and stub is not necessary.
>>
>> But that means we'd need to add "#include CONFIG_DEVICES" in
>> a sysemu/ header, which looks like an anti-pattern.
>>
>> Thomas / Paolo, what do you think?
> 
> I'd only do that if you are very, very sure that the header file is 
> only included from target-specific files. Otherwise this will of course
> cause more trouble than benefit.

Hmm it could be clearer to rearrange the target-specific sysemu/
headers. For this example, eventually sysemu/i386/sev.h?

Phil.


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Philippe Mathieu-Daudé 2 years, 9 months ago
On 6/21/21 11:15 AM, Philippe Mathieu-Daudé wrote:
> On 6/21/21 10:44 AM, Thomas Huth wrote:
>> On 17/06/2021 17.48, Philippe Mathieu-Daudé wrote:
>> [...]
>>> This works, but I'd rather use:
>>>
>>>    if (sev_enabled()) {
>>>        sev_kernel_loader_calc_cmdline_hash(&sev_loader_context,
>>>                                            kernel_cmdline);
>>>    }
>>>
>>> And have sev_enabled() defined as:
>>>
>>> #ifdef CONFIG_SEV
>>> bool sev_enabled(void);
>>> #else
>>> #define sev_enabled() false
>>> #endif
>>>
>>> So the compiler could elide the statement if SEV is disabled,
>>> and stub is not necessary.
>>>
>>> But that means we'd need to add "#include CONFIG_DEVICES" in
>>> a sysemu/ header, which looks like an anti-pattern.
>>>
>>> Thomas / Paolo, what do you think?
>>
>> I'd only do that if you are very, very sure that the header file is 
>> only included from target-specific files. Otherwise this will of course
>> cause more trouble than benefit.

Back to Paolo, I think the problem is we started to use target-specific
features in Kconfig, which was designed for devices (not
target-specific). We have the same problem (another thread) with the
semihosting architectural feature.

> Hmm it could be clearer to rearrange the target-specific sysemu/
> headers. For this example, eventually sysemu/i386/sev.h?
> 
> Phil.
> 


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Eduardo Habkost 2 years, 9 months ago
On Thu, Jun 17, 2021 at 03:48:52PM +0300, Dov Murik wrote:
> 
> 
> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
> > Hi Dov, James,
> > 
> > +Connor who asked to be reviewer.
> > 
> > On 6/15/21 5:20 PM, Eduardo Habkost wrote:
> >> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
> >>> From: James Bottomley <jejb@linux.ibm.com>
> >>>
> >>> If the VM is using memory encryption and also specifies a kernel/initrd
> >>> or appended command line, calculate the hashes and add them to the
> >>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
> >>> in the kernel/initrd/cmdline via the fw_cfg interface).
> >>>
> >>> 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 memcrypt
> >>> interface, the hash of the encrypted data will be accumulated by the
> >>> PSP.
> >>>
> >>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> >>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> >>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
> >>> strings, remove GCC pragma, fix checkpatch errors]
> >>> ---
> >>>
> >>> OVMF support for handling the table of hashes (verifying that the
> >>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> >>> to the measured hashes in the table) will be posted soon to edk2-devel.
> >>>
> >>> ---
> >>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 119 insertions(+), 1 deletion(-)
> >>>
> >>
> >> This is not an objection to the patch itself, but: can we do
> >> something to move all sev-related code to sev.c?  It would make
> >> the process of assigning a maintainer and reviewing/merging
> >> future patches much simpler.
> >>
> >> I am not familiar with SEV internals, so my only question is
> >> about configurations where SEV is disabled:
> >>
> >> [...]
> >>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> >>> @@ -778,6 +818,11 @@ 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;
> >>> +    uint8_t buf[HASH_SIZE];
> >>> +    uint8_t *hash = buf;
> >>> +    size_t hash_len = sizeof(buf);
> >>> +    struct sev_hash_table *sev_ht = NULL;
> >>> +    int sev_ht_index = 0;
> > 
> > Can you move all these variable into a structure, and use it as a
> > SEV loader context?
> > 
> > Then each block of code you added can be moved to its own function,
> > self-described, working with the previous context.
> > 
> > The functions can be declared in sev_i386.h and defined in sev.c as
> > Eduardo suggested.
> > 
> 
> Thanks Philippe, I'll try this approach.
> 
> 
> I assume you mean that an addition like this:
> 
> +    if (sev_ht) {
> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> +
> +        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> +                           strlen(kernel_cmdline) + 1,
> +                           &hash, &hash_len, &error_fatal);
> +        memcpy(e->hash, hash, hash_len);
> +        e->len = sizeof(*e);
> +        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> +    }
> 
> will be extracted to a function, and here (in x86_load_linux()) replaced
> with a single call:
> 
>     sev_kernel_loader_calc_cmdline_hash(&sev_loader_context, kernel_cmdline);
>   
> and that function will have an empty stub in non-SEV builds, and a do-
> nothing condition (at the beginning) if it's an SEV-disabled VM, and
> will do the actual work in SEV VMs.
> 
> Right?

I would suggest a generic notification mechanism instead, where
SEV code could register to be notified after the kernel/initrd is
loaded.

Maybe the existing qemu_add_machine_init_done_notifier()
mechanism would be enough for this?  Is there a reason the hash
calculation needs to be done inside x86_load_linux(),
specifically?

> 
> 
> Also, should I base my next version on top of the current master, or on
> top of your SEV-Housekeeping patch series, or on top of some other tree?
> 
> 
> -Dov
> 
> >>>  
> >>>      /* Align to 16 bytes as a paranoia measure */
> >>>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> >>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
> >>>          exit(1);
> >>>      }
> >>>  
> >>> +    if (machine->cgs && machine->cgs->ready) {
> >>
> >> machine->cgs doesn't seem to be a SEV-specific field.
> >> What if machine->cgs->ready is set but SEV is disabled?
> >>
> >>> +        uint8_t *data;
> >>> +        struct sev_hash_table_descriptor *area;
> >>> +
> >>> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> >>> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
> >>> +                    "no hash table guid\n");
> >>> +            exit(1);
> >>> +        }
> >>> +        area = (struct sev_hash_table_descriptor *)data;
> >>> +
> >>> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
> >>> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
> >>> +        sev_ht->len = sizeof(*sev_ht);
> >>> +    }
> >>> +
> >>>      /* kernel protocol version */
> >>>      if (ldl_p(header + 0x202) == 0x53726448) {
> >>>          protocol = lduw_p(header + 0x206);
> >> [...]
> >>
> > 
> 

-- 
Eduardo


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Dov Murik 2 years, 9 months ago

On 17/06/2021 20:22, Eduardo Habkost wrote:
> On Thu, Jun 17, 2021 at 03:48:52PM +0300, Dov Murik wrote:
>>
>>
>> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
>>> Hi Dov, James,
>>>
>>> +Connor who asked to be reviewer.
>>>
>>> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
>>>> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
>>>>> From: James Bottomley <jejb@linux.ibm.com>
>>>>>
>>>>> If the VM is using memory encryption and also specifies a kernel/initrd
>>>>> or appended command line, calculate the hashes and add them to the
>>>>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
>>>>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>>>>
>>>>> 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 memcrypt
>>>>> interface, the hash of the encrypted data will be accumulated by the
>>>>> PSP.
>>>>>
>>>>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>>>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>>>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
>>>>> strings, remove GCC pragma, fix checkpatch errors]
>>>>> ---
>>>>>
>>>>> OVMF support for handling the table of hashes (verifying that the
>>>>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>>>>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>>>>
>>>>> ---
>>>>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 119 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> This is not an objection to the patch itself, but: can we do
>>>> something to move all sev-related code to sev.c?  It would make
>>>> the process of assigning a maintainer and reviewing/merging
>>>> future patches much simpler.
>>>>
>>>> I am not familiar with SEV internals, so my only question is
>>>> about configurations where SEV is disabled:
>>>>
>>>> [...]
>>>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>>>> @@ -778,6 +818,11 @@ 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;
>>>>> +    uint8_t buf[HASH_SIZE];
>>>>> +    uint8_t *hash = buf;
>>>>> +    size_t hash_len = sizeof(buf);
>>>>> +    struct sev_hash_table *sev_ht = NULL;
>>>>> +    int sev_ht_index = 0;
>>>
>>> Can you move all these variable into a structure, and use it as a
>>> SEV loader context?
>>>
>>> Then each block of code you added can be moved to its own function,
>>> self-described, working with the previous context.
>>>
>>> The functions can be declared in sev_i386.h and defined in sev.c as
>>> Eduardo suggested.
>>>
>>
>> Thanks Philippe, I'll try this approach.
>>
>>
>> I assume you mean that an addition like this:
>>
>> +    if (sev_ht) {
>> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
>> +
>> +        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
>> +                           strlen(kernel_cmdline) + 1,
>> +                           &hash, &hash_len, &error_fatal);
>> +        memcpy(e->hash, hash, hash_len);
>> +        e->len = sizeof(*e);
>> +        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
>> +    }
>>
>> will be extracted to a function, and here (in x86_load_linux()) replaced
>> with a single call:
>>
>>     sev_kernel_loader_calc_cmdline_hash(&sev_loader_context, kernel_cmdline);
>>   
>> and that function will have an empty stub in non-SEV builds, and a do-
>> nothing condition (at the beginning) if it's an SEV-disabled VM, and
>> will do the actual work in SEV VMs.
>>
>> Right?
> 
> I would suggest a generic notification mechanism instead, where
> SEV code could register to be notified after the kernel/initrd is
> loaded.
> 
> Maybe the existing qemu_add_machine_init_done_notifier()
> mechanism would be enough for this?  Is there a reason the hash
> calculation needs to be done inside x86_load_linux(),
> specifically?
> 

SEV already registers that callback - sev_machine_done_notify, which
calls sev_launch_get_measure.  We want the hashes page filled and
encrypted *before* that measurement is taken by the PSP.  We can squeeze
in the hashes and page encryption before the measurement *if* we can
calculate the hashes at this point.

x86_load_linux already deals with kernel/initrd/cmdline, so that was the
easiest place to add the hash calculation.

It's also a bit weird (semantically) to modify the guest RAM after
pc_memory_init has finished.


We can add a new notifier towards the end of x86_load_linux, but can we
avoid passing all the different pointers+sizes of kernel/initrd/cmdline
to that notifier?  Do you envision other uses for registering on that event?


-Dov


>>
>>
>> Also, should I base my next version on top of the current master, or on
>> top of your SEV-Housekeeping patch series, or on top of some other tree?
>>
>>
>> -Dov
>>
>>>>>  
>>>>>      /* Align to 16 bytes as a paranoia measure */
>>>>>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>>>>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>>>>>          exit(1);
>>>>>      }
>>>>>  
>>>>> +    if (machine->cgs && machine->cgs->ready) {
>>>>
>>>> machine->cgs doesn't seem to be a SEV-specific field.
>>>> What if machine->cgs->ready is set but SEV is disabled?
>>>>
>>>>> +        uint8_t *data;
>>>>> +        struct sev_hash_table_descriptor *area;
>>>>> +
>>>>> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>>>>> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
>>>>> +                    "no hash table guid\n");
>>>>> +            exit(1);
>>>>> +        }
>>>>> +        area = (struct sev_hash_table_descriptor *)data;
>>>>> +
>>>>> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
>>>>> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
>>>>> +        sev_ht->len = sizeof(*sev_ht);
>>>>> +    }
>>>>> +
>>>>>      /* kernel protocol version */
>>>>>      if (ldl_p(header + 0x202) == 0x53726448) {
>>>>>          protocol = lduw_p(header + 0x206);
>>>> [...]
>>>>
>>>
>>
> 

Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Eduardo Habkost 2 years, 9 months ago
On Thu, Jun 17, 2021 at 3:17 PM Dov Murik <dovmurik@linux.ibm.com> wrote:
>
>
>
> On 17/06/2021 20:22, Eduardo Habkost wrote:
> > On Thu, Jun 17, 2021 at 03:48:52PM +0300, Dov Murik wrote:
> >>
> >>
> >> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
> >>> Hi Dov, James,
> >>>
> >>> +Connor who asked to be reviewer.
> >>>
> >>> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
> >>>> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
> >>>>> From: James Bottomley <jejb@linux.ibm.com>
> >>>>>
> >>>>> If the VM is using memory encryption and also specifies a kernel/initrd
> >>>>> or appended command line, calculate the hashes and add them to the
> >>>>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
> >>>>> in the kernel/initrd/cmdline via the fw_cfg interface).
> >>>>>
> >>>>> 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 memcrypt
> >>>>> interface, the hash of the encrypted data will be accumulated by the
> >>>>> PSP.
> >>>>>
> >>>>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> >>>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> >>>>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
> >>>>> strings, remove GCC pragma, fix checkpatch errors]
> >>>>> ---
> >>>>>
> >>>>> OVMF support for handling the table of hashes (verifying that the
> >>>>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> >>>>> to the measured hashes in the table) will be posted soon to edk2-devel.
> >>>>>
> >>>>> ---
> >>>>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>>>  1 file changed, 119 insertions(+), 1 deletion(-)
> >>>>>
> >>>>
> >>>> This is not an objection to the patch itself, but: can we do
> >>>> something to move all sev-related code to sev.c?  It would make
> >>>> the process of assigning a maintainer and reviewing/merging
> >>>> future patches much simpler.
> >>>>
> >>>> I am not familiar with SEV internals, so my only question is
> >>>> about configurations where SEV is disabled:
> >>>>
> >>>> [...]
> >>>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> >>>>> @@ -778,6 +818,11 @@ 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;
> >>>>> +    uint8_t buf[HASH_SIZE];
> >>>>> +    uint8_t *hash = buf;
> >>>>> +    size_t hash_len = sizeof(buf);
> >>>>> +    struct sev_hash_table *sev_ht = NULL;
> >>>>> +    int sev_ht_index = 0;
> >>>
> >>> Can you move all these variable into a structure, and use it as a
> >>> SEV loader context?
> >>>
> >>> Then each block of code you added can be moved to its own function,
> >>> self-described, working with the previous context.
> >>>
> >>> The functions can be declared in sev_i386.h and defined in sev.c as
> >>> Eduardo suggested.
> >>>
> >>
> >> Thanks Philippe, I'll try this approach.
> >>
> >>
> >> I assume you mean that an addition like this:
> >>
> >> +    if (sev_ht) {
> >> +        struct sev_hash_table_entry *e = &sev_ht->entries[sev_ht_index++];
> >> +
> >> +        qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> >> +                           strlen(kernel_cmdline) + 1,
> >> +                           &hash, &hash_len, &error_fatal);
> >> +        memcpy(e->hash, hash, hash_len);
> >> +        e->len = sizeof(*e);
> >> +        memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> >> +    }
> >>
> >> will be extracted to a function, and here (in x86_load_linux()) replaced
> >> with a single call:
> >>
> >>     sev_kernel_loader_calc_cmdline_hash(&sev_loader_context, kernel_cmdline);
> >>
> >> and that function will have an empty stub in non-SEV builds, and a do-
> >> nothing condition (at the beginning) if it's an SEV-disabled VM, and
> >> will do the actual work in SEV VMs.
> >>
> >> Right?
> >
> > I would suggest a generic notification mechanism instead, where
> > SEV code could register to be notified after the kernel/initrd is
> > loaded.
> >
> > Maybe the existing qemu_add_machine_init_done_notifier()
> > mechanism would be enough for this?  Is there a reason the hash
> > calculation needs to be done inside x86_load_linux(),
> > specifically?
> >
>
> SEV already registers that callback - sev_machine_done_notify, which
> calls sev_launch_get_measure.  We want the hashes page filled and
> encrypted *before* that measurement is taken by the PSP.  We can squeeze
> in the hashes and page encryption before the measurement *if* we can
> calculate the hashes at this point.
>
> x86_load_linux already deals with kernel/initrd/cmdline, so that was the
> easiest place to add the hash calculation.
>
> It's also a bit weird (semantically) to modify the guest RAM after
> pc_memory_init has finished.

Understood.

>
>
> We can add a new notifier towards the end of x86_load_linux, but can we
> avoid passing all the different pointers+sizes of kernel/initrd/cmdline
> to that notifier?  Do you envision other uses for registering on that event?

I expect other confidential guest technologies to do something very
similar, but I don't want to make you overengineer this. The generic
mechanism was just a suggestion.

Extracting the code to a single SEV-specific function would already be
an improvement. That would make the code easier to refactor if we
decide we need a generic mechanism in the future.

-- 
Eduardo


Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Posted by Dov Murik 2 years, 9 months ago
Hi Eduardo,

On 15/06/2021 18:20, Eduardo Habkost wrote:
> On Tue, May 25, 2021 at 06:59:31AM +0000, Dov Murik wrote:
>> From: James Bottomley <jejb@linux.ibm.com>
>>
>> If the VM is using memory encryption and also specifies a kernel/initrd
>> or appended command line, calculate the hashes and add them to the
>> encrypted data.  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 (if the GUID doesn't exist, the user isn't allowed to pass
>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>
>> 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 memcrypt
>> interface, the hash of the encrypted data will be accumulated by the
>> PSP.
>>
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> [dovmurik@linux.ibm.com: use machine->cgs, remove parsing of GUID
>> strings, remove GCC pragma, fix checkpatch errors]
>> ---
>>
>> OVMF support for handling the table of hashes (verifying that the
>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>
>> ---
>>  hw/i386/x86.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 119 insertions(+), 1 deletion(-)
>>
> 
> This is not an objection to the patch itself, but: can we do
> something to move all sev-related code to sev.c?  It would make
> the process of assigning a maintainer and reviewing/merging
> future patches much simpler.
> 

I'll look into this following Philippe's suggestions.

> I am not familiar with SEV internals, so my only question is
> about configurations where SEV is disabled:
> 
> [...]
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> @@ -778,6 +818,11 @@ 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;
>> +    uint8_t buf[HASH_SIZE];
>> +    uint8_t *hash = buf;
>> +    size_t hash_len = sizeof(buf);
>> +    struct sev_hash_table *sev_ht = NULL;
>> +    int sev_ht_index = 0;
>>  
>>      /* Align to 16 bytes as a paranoia measure */
>>      cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>>          exit(1);
>>      }
>>  
>> +    if (machine->cgs && machine->cgs->ready) {
> 
> machine->cgs doesn't seem to be a SEV-specific field.
> What if machine->cgs->ready is set but SEV is disabled?
> 

You're right; I'll change this to sev_enabled() like in
hw/i386/pc_sysfw.c .

-Dov


>> +        uint8_t *data;
>> +        struct sev_hash_table_descriptor *area;
>> +
>> +        if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>> +            fprintf(stderr, "qemu: kernel command line specified but OVMF has "
>> +                    "no hash table guid\n");
>> +            exit(1);
>> +        }
>> +        area = (struct sev_hash_table_descriptor *)data;
>> +
>> +        sev_ht = qemu_map_ram_ptr(NULL, area->base);
>> +        memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
>> +        sev_ht->len = sizeof(*sev_ht);
>> +    }
>> +
>>      /* kernel protocol version */
>>      if (ldl_p(header + 0x202) == 0x53726448) {
>>          protocol = lduw_p(header + 0x206);
> [...]
>