| Message ID | 20230207224847.94429-3-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: fix fallout from switching setup_data from kernel image to cmdline | expand |
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);
On Wed, Feb 8, 2023 at 12:38 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: > > 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. Yes, it does make sense to do it like this 2/2 patch does here. (I made the change for the DTB.) The reason is that the first setup_data link is already conditionalized on sev: /* * 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() && first_setup_data) { SetupDataFixup *fixup = g_malloc(sizeof(*fixup)); memcpy(setup, header, MIN(sizeof(header), setup_size)); /* Offset 0x250 is a pointer to the first setup_data link. */
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()) { 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);