Re: [PATCH v2 1/2] x86: only modify setup_data if the boot protocol indicates safety

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Sep 06, 2022 at 01:14:50PM +0200, Ard Biesheuvel wrote:
> (cc Laszlo)
> 
> On Tue, 6 Sept 2022 at 12:45, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Sep 06, 2022 at 12:43:55PM +0200, Jason A. Donenfeld wrote:
> > > On Tue, Sep 6, 2022 at 12:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Sep 06, 2022 at 12:36:56PM +0200, Jason A. Donenfeld wrote:
> > > > > It's only safe to modify the setup_data pointer on newer kernels where
> > > > > the EFI stub loader will ignore it. So condition setting that offset on
> > > > > the newer boot protocol version. While we're at it, gate this on SEV too.
> > > > > This depends on the kernel commit linked below going upstream.
> > > > >
> > > > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > > > Cc: Laurent Vivier <laurent@vivier.eu>
> > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > > > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > > > Cc: Richard Henderson <richard.henderson@linaro.org>
> > > > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > > > Link: https://lore.kernel.org/linux-efi/20220904165321.1140894-1-Jason@zx2c4.com/
> > > > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > > >
> > > > BTW what does it have to do with SEV?
> > > > Is this because SEV is not going to trust the data to be random anyway?
> > >
> > > Daniel (now CC'd) pointed out in one of the previous threads that this
> > > breaks SEV, because the image hash changes.
> > >
> > > Jason
> >
> > Oh I see. I'd add a comment maybe and definitely mention this
> > in the commit log.
> >
> 
> This does raise the question (as I mentioned before) how things like
> secure boot and measured boot are affected when combined with direct
> kernel boot: AIUI, libvirt uses direct kernel boot at guest
> installation time, and modifying setup_data will corrupt the image
> signature.

IIUC, qemu already modifies setup_data when using direct kernel boot.

It put in logic to skip this if SEV is enabled, to avoid interfering
with SEV hashes over the kernel, but there's nothing doing this more
generally for non-SEV cases using UEFI. So potentially use of SecureBoot
may already be impacted when using direct kernel boot. I haven't formally
tested this myself though. I just saw that earlier versions of this
RNG patch broke SEV hashes and later versions addressed that problem
for SEV when the code was re-arranged.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|