| Message ID | 20210621190553.1763020-3-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline | expand |
On 6/21/21 2:05 PM, Dov Murik wrote: > If SEV is enabled and a kernel is passed via -kernel, pass the hashes of > kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV > measured boot. > > Co-developed-by: James Bottomley <jejb@linux.ibm.com> > Signed-off-by: James Bottomley <jejb@linux.ibm.com> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > --- > hw/i386/x86.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index ed796fe6ba..5c46463d9f 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -45,6 +45,7 @@ > #include "hw/i386/fw_cfg.h" > #include "hw/intc/i8259.h" > #include "hw/rtc/mc146818rtc.h" > +#include "target/i386/sev_i386.h" > > #include "hw/acpi/cpu_hotplug.h" > #include "hw/irq.h" > @@ -778,6 +779,7 @@ void x86_load_linux(X86MachineState *x86ms, > const char *initrd_filename = machine->initrd_filename; > const char *dtb_filename = machine->dtb; > const char *kernel_cmdline = machine->kernel_cmdline; > + KernelLoaderContext kernel_loader_context = {}; > > /* Align to 16 bytes as a paranoia measure */ > cmdline_size = (strlen(kernel_cmdline) + 16) & ~15; > @@ -924,6 +926,8 @@ void x86_load_linux(X86MachineState *x86ms, > fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr); > fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1); > fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline); > + kernel_loader_context.cmdline_data = (char *)kernel_cmdline; > + kernel_loader_context.cmdline_size = strlen(kernel_cmdline) + 1; I just wanted to check my understanding: I'm guessing you didn't set `kernel_loader_context.cmdline_size` to `cmdline_size` (defined above) so guest owners don't have to be aware of whatever alignment precaution QEMU takes when producing their own measurement, right? Otherwise: Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
Hi Connor, On 22/06/2021 23:55, Connor Kuehl wrote: > On 6/21/21 2:05 PM, Dov Murik wrote: >> If SEV is enabled and a kernel is passed via -kernel, pass the hashes of >> kernel/initrd/cmdline in an encrypted guest page to OVMF for SEV >> measured boot. >> >> Co-developed-by: James Bottomley <jejb@linux.ibm.com> >> Signed-off-by: James Bottomley <jejb@linux.ibm.com> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >> --- >> hw/i386/x86.c | 25 ++++++++++++++++++++++++- >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >> index ed796fe6ba..5c46463d9f 100644 >> --- a/hw/i386/x86.c >> +++ b/hw/i386/x86.c >> @@ -45,6 +45,7 @@ >> #include "hw/i386/fw_cfg.h" >> #include "hw/intc/i8259.h" >> #include "hw/rtc/mc146818rtc.h" >> +#include "target/i386/sev_i386.h" >> >> #include "hw/acpi/cpu_hotplug.h" >> #include "hw/irq.h" >> @@ -778,6 +779,7 @@ void x86_load_linux(X86MachineState *x86ms, >> const char *initrd_filename = machine->initrd_filename; >> const char *dtb_filename = machine->dtb; >> const char *kernel_cmdline = machine->kernel_cmdline; >> + KernelLoaderContext kernel_loader_context = {}; >> >> /* Align to 16 bytes as a paranoia measure */ >> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15; >> @@ -924,6 +926,8 @@ void x86_load_linux(X86MachineState *x86ms, >> fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr); >> fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1); >> fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline); >> + kernel_loader_context.cmdline_data = (char *)kernel_cmdline; >> + kernel_loader_context.cmdline_size = strlen(kernel_cmdline) + 1; > > I just wanted to check my understanding: I'm guessing you didn't set > `kernel_loader_context.cmdline_size` to `cmdline_size` (defined above) > so guest owners don't have to be aware of whatever alignment precaution > QEMU takes when producing their own measurement, right? The hashes calculated by OVMF must be identical to the hashes calculated by QEMU. OVMF calculates a hash over the blob it receives from QEMU, which is: fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1); fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline); That is -- the entire cmdline string including the terminating '\0'. Here's the OVMF side that verifies the last byte is '\0': QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData); QemuFwCfgReadBytes (CommandLineSize, CommandLine); // // Verify NUL-termination of the command line. // if (CommandLine[CommandLineSize - 1] != '\0') { DEBUG ((DEBUG_ERROR, "%a: kernel command line is not NUL-terminated\n", __FUNCTION__)); Status = EFI_PROTOCOL_ERROR; goto FreeCommandLine; } (this is more or less where we're adding the hash verification in OVMF.) As you said, the Guest Owner also calculates the hash in exact the same way in order to verify the SEV PSP measurement. As I understand from the x86_load_linux code, the rounded/aligned `cmdline_size` is used for placing the command line in an aligned address inside the guest memory. In the kernel I'm booting the `cmdline_addr` is always 0x20000 and doesn't depend on cmdline_size at all. > > Otherwise: > > Reviewed-by: Connor Kuehl <ckuehl@redhat.com> > Thanks! -Dov
diff --git a/hw/i386/x86.c b/hw/i386/x86.c index ed796fe6ba..5c46463d9f 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -45,6 +45,7 @@ #include "hw/i386/fw_cfg.h" #include "hw/intc/i8259.h" #include "hw/rtc/mc146818rtc.h" +#include "target/i386/sev_i386.h" #include "hw/acpi/cpu_hotplug.h" #include "hw/irq.h" @@ -778,6 +779,7 @@ void x86_load_linux(X86MachineState *x86ms, const char *initrd_filename = machine->initrd_filename; const char *dtb_filename = machine->dtb; const char *kernel_cmdline = machine->kernel_cmdline; + KernelLoaderContext kernel_loader_context = {}; /* Align to 16 bytes as a paranoia measure */ cmdline_size = (strlen(kernel_cmdline) + 16) & ~15; @@ -924,6 +926,8 @@ void x86_load_linux(X86MachineState *x86ms, fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_ADDR, cmdline_addr); fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1); fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline); + kernel_loader_context.cmdline_data = (char *)kernel_cmdline; + kernel_loader_context.cmdline_size = strlen(kernel_cmdline) + 1; if (protocol >= 0x202) { stl_p(header + 0x228, cmdline_addr); @@ -1005,6 +1009,8 @@ void x86_load_linux(X86MachineState *x86ms, fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr); fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data, initrd_size); + kernel_loader_context.initrd_data = initrd_data; + kernel_loader_context.initrd_size = initrd_size; stl_p(header + 0x218, initrd_addr); stl_p(header + 0x21c, initrd_size); @@ -1063,15 +1069,32 @@ void x86_load_linux(X86MachineState *x86ms, load_image_size(dtb_filename, setup_data->data, dtb_size); } - memcpy(setup, header, MIN(sizeof(header), setup_size)); + /* + * If we're starting an encrypted VM, it will be OVMF based, which uses the + * efi stub for booting and doesn't require any values to be placed in the + * kernel header. We therefore don't update the header so the hash of the + * kernel on the other side of the fw_cfg interface matches the hash of the + * file the user passed in. + */ + if (!sev_enabled()) { + memcpy(setup, header, MIN(sizeof(header), setup_size)); + } fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr); fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size); + kernel_loader_context.kernel_data = (char *)kernel; + kernel_loader_context.kernel_size = kernel_size; fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr); fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size); + kernel_loader_context.setup_data = (char *)setup; + kernel_loader_context.setup_size = setup_size; + + if (sev_enabled()) { + sev_add_kernel_loader_hashes(&kernel_loader_context, &error_fatal); + } option_rom[nb_option_roms].bootindex = 0; option_rom[nb_option_roms].name = "linuxboot.bin";