| Message ID | 20210525065931.1628554-1-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline | expand |
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
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)) { >
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); [...]
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); > [...] >
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); > [...] >
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); >> [...] >> >
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.
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); > >> [...] > >> > > >
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); >>>> [...] >>>> >>> >> >
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.
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
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.
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. >
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
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
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!
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)) {