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

[Dov Murik <dovmurik@linux.ibm.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