Re: [PATCH v2] hw/i386: place setup_data at fixed place in memory

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>,
	"Laszlo Ersek" <lersek@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Xiaoyao Li" <xiaoyao.li@intel.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	linux-efi <linux-efi@vger.kernel.org>
Subject: Re: [PATCH v2] hw/i386: place setup_data at fixed place in memory
Date: Thu, 4 Aug 2022 14:28:51 +0200	[thread overview]
Message-ID: <CAHmME9pydxka29rQp_o4Sqss_+4VGMj09F_JACxAFMaq9sM36g@mail.gmail.com> (raw)
In-Reply-To: <CAHmME9pxrFpGWXiubxU9r2sDMV-P+QuDiZvw4OaSZvWOkDbyTA@mail.gmail.com>

On Thu, Aug 4, 2022 at 2:17 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Ard,
>
> On Thu, Aug 4, 2022 at 2:16 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 4 Aug 2022 at 14:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Thu, Aug 04, 2022 at 02:03:29PM +0200, Jason A. Donenfeld wrote:
> > > > Hi Daniel,
> > > >
> > > > On Thu, Aug 04, 2022 at 10:25:36AM +0100, Daniel P. Berrangé wrote:
> > > > > Yep, and ultimately the inability to distinguish UEFI vs other firmware
> > > > > is arguably correct by design, as the QEMU <-> firmware interface is
> > > > > supposed to be arbitrarily pluggable for any firmware implementation
> > > > > not  limited to merely UEFI + seabios.
> > > >
> > > > Indeed, I agree with this.
> > > >
> > > > >
> > > > > > For now I suggest either reverting the original patch, or at least not
> > > > > > enabling the knob by default for any machine types. In particular, when
> > > > > > using MicroVM, the user must leave the knob disabled when direct booting
> > > > > > a kernel on OVMF, and the user may or may not enable the knob when
> > > > > > direct booting a kernel on SeaBIOS.
> > > > >
> > > > > Having it opt-in via a knob would defeat Jason's goal of having the seed
> > > > > available automatically.
> > > >
> > > > Yes, adding a knob is absolutely out of the question.
> > > >
> > > > It also doesn't actually solve the problem: this triggers when QEMU
> > > > passes a DTB too. It's not just for the new RNG seed thing. This bug
> > > > isn't new.
> > >
> > > In the other thread I also mentioned that this RNG Seed addition has
> > > caused a bug with AMD SEV too, making boot measurement attestation
> > > fail because the kernel blob passed to the firmware no longer matches
> > > what the tenant expects, due to the injected seed.
> > >
> >
> > I was actually expecting this to be an issue in the
> > signing/attestation department as well, and you just confirmed my
> > suspicion.
> >
> > But does this mean that populating the setup_data pointer is out of
> > the question altogether? Or only that putting the setup_data linked
> > list nodes inside the image is a problem?
>
> If you look at the v2 patch, populating boot_param->setup_data winds
> up being a fixed value. So even if that part was a problem (though I
> don't think it is), it won't be with the v2 patch, since it's always
> the same.

Actually, `setup` isn't even modified if SEV is being used anyway. So
really, the approach of this v2 -- of not modifying the kernel image
-- should fix that issue, no matter what.

Jason