On Wed, Feb 8, 2023 at 12:38 PM Tom Lendacky <thomas.lenda...@amd.com> wrote:
>
> On 2/7/23 16:48, Jason A. Donenfeld wrote:
> > From: Dov Murik <dovmu...@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.lenda...@amd.com>
> > Signed-off-by: Dov Murik <dovmu...@linux.ibm.com>
> > Signed-off-by: Jason A. Donenfeld <ja...@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. */
