Re: [PATCH 2/2] x86: don't append setup_data to cmdline for SEV guests

[Tom Lendacky]

On 2/7/23 16:48, Jason A. Donenfeld wrote:
> From: Dov Murik <dovmurik@linux.ibm.com>
>
> Modifying the cmdline by appending setup_data breaks measured boot with
> SEV and OVMF, and possibly signed boot. Previously this was disabled
> when appending to the kernel image, but with eac7a7791bb6 ("x86: don't
> let decompressed kernel image clobber setup_data"), this was changed to
> the cmdline file instead, with the sev_enabled() check left out.
>
> Fixes: eac7a7791bb6 ("x86: don't let decompressed kernel image clobber 
> setup_data")
> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>   hw/i386/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index c6d7bf6db2..80a1678acd 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1079,7 +1079,7 @@ void x86_load_linux(X86MachineState *x86ms,
>       fclose(f);
>   
>       /* append dtb to kernel */
> -    if (dtb_filename) {
> +    if (dtb_filename && !sev_enabled()) {

Is this change necessary? From an SEV point of view, the DTB file should 
be "constant" and so a guest owner will be able to use that to correctly 
verify the SEV LAUNCH measurement. Additionally, it won't change from the 
time it was measured to the time OVMF validates everything. Of course, I 
don't really anticipate that a DTB file would be used with an SEV guest, 
anyway.

@Dov, does that make sense?

Thanks,
Tom

>           if (protocol < 0x209) {
>               fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
>               exit(1);
> @@ -1103,7 +1103,7 @@ void x86_load_linux(X86MachineState *x86ms,
>           load_image_size(dtb_filename, setup_data->data, dtb_size);
>       }
>   
> -    if (!legacy_no_rng_seed && protocol >= 0x209) {
> +    if (!legacy_no_rng_seed && protocol >= 0x209 && !sev_enabled()) {
>           setup_data_offset = cmdline_size;
>           cmdline_size += sizeof(SetupData) + RNG_SEED_LENGTH;
>           kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size);