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

[Jason A. Donenfeld]

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. */