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

[Daniel P . Berrangé]

On Thu, Sep 08, 2022 at 02:28:29PM +0200, Ard Biesheuvel wrote:
> On Thu, 8 Sept 2022 at 13:30, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 09/06/22 13:33, Daniel P. Berrangé wrote:
> > > 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.
> >
> > Yes,
> >
> > https://github.com/tianocore/edk2/commit/82808b422617
> >
> 
> Ah yes, thanks for jogging my memory.
> 
> So virt-install --network already ignores secure boot failures on
> direct kernel boot, so this is not going to make it any worse.

And in a cloud world this isn't too much of a problem to start
with. The cloud disks images will be built offline in trusted
infrastructure, so lack of SecureBoot isn't a show stopper. When
later deployed to the public cloud, SecureBoot (and/or Confidential
Boot) will be fully operational, where it matters most.

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 :|