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

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

[Michael S. Tsirkin via]

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?

> ---
>  hw/i386/x86.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 050eedc0c8..fddc20df03 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1088,8 +1088,15 @@ void x86_load_linux(X86MachineState *x86ms,
>          qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
>      }
>  
> -    /* Offset 0x250 is a pointer to the first setup_data link. */
> -    stq_p(header + 0x250, first_setup_data);
> +    /*
> +     * Only modify the header if doing so won't crash EFI boot, which is the
> +     * case only for newer boot protocols, and don't do so either if SEV is
> +     * enabled.
> +     */
> +    if (protocol >= 0x210 && !sev_enabled()) {
> +        /* Offset 0x250 is a pointer to the first setup_data link. */
> +        stq_p(header + 0x250, first_setup_data);
> +    }
>  
>      /*
>       * If we're starting an encrypted VM, it will be OVMF based, which uses the
> -- 
> 2.37.3

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

[Jason A. Donenfeld]

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

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

[Michael S. Tsirkin]

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.

-- 
MST

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

[Jason A. Donenfeld]

On Tue, Sep 06, 2022 at 06:45:34AM -0400, Michael S. Tsirkin 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.

Sure, will do.

Jason

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

[Jason A. Donenfeld]

On Tue, Sep 06, 2022 at 12:46:32PM +0200, Jason A. Donenfeld wrote:
> On Tue, Sep 06, 2022 at 06:45:34AM -0400, Michael S. Tsirkin 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.
> 
> Sure, will do.

Actually, I'm wrong. This is already done implicitly below with a huge
comment:

    /*
     * 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()) {
        memcpy(setup, header, MIN(sizeof(header), setup_size));
    }

So I'll remove the SEV bit from this commit. Whoops. Glad you asked
about it.

Jason

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

[Jason A. Donenfeld]

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.

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/
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 hw/i386/x86.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 050eedc0c8..0c355c29b4 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1088,8 +1088,14 @@ void x86_load_linux(X86MachineState *x86ms,
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
     }
 
-    /* Offset 0x250 is a pointer to the first setup_data link. */
-    stq_p(header + 0x250, first_setup_data);
+    /*
+     * Only modify the header if doing so won't crash EFI boot, which is the
+     * case only for newer boot protocols.
+     */
+    if (protocol >= 0x210) {
+        /* Offset 0x250 is a pointer to the first setup_data link. */
+        stq_p(header + 0x250, first_setup_data);
+    }
 
     /*
      * If we're starting an encrypted VM, it will be OVMF based, which uses the
-- 
2.37.3

[PATCH v3 2/2] x86: re-enable rng seeding via setup_data

[Jason A. Donenfeld]

This reverts 3824e25db1 ("x86: disable rng seeding via setup_data"), but
for 7.2 rather than 7.1, now that modifying setup_data is safe to do.

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>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 hw/i386/microvm.c | 2 +-
 hw/i386/pc_piix.c | 3 ++-
 hw/i386/pc_q35.c  | 3 ++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 52cafa003d..7fe8cce03e 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -332,7 +332,7 @@ static void microvm_memory_init(MicrovmMachineState *mms)
     rom_set_fw(fw_cfg);
 
     if (machine->kernel_filename != NULL) {
-        x86_load_linux(x86ms, fw_cfg, 0, true, true);
+        x86_load_linux(x86ms, fw_cfg, 0, true, false);
     }
 
     if (mms->option_roms) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8043a250ad..0b1a79c0fa 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -439,7 +439,6 @@ static void pc_i440fx_7_2_machine_options(MachineClass *m)
     m->alias = "pc";
     m->is_default = true;
     pcmc->default_cpu_version = 1;
-    pcmc->legacy_no_rng_seed = true;
 }
 
 DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
@@ -447,9 +446,11 @@ DEFINE_I440FX_MACHINE(v7_2, "pc-i440fx-7.2", NULL,
 
 static void pc_i440fx_7_1_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_7_2_machine_options(m);
     m->alias = NULL;
     m->is_default = false;
+    pcmc->legacy_no_rng_seed = true;
     compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
     compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 53eda50e81..a496bd6e74 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -376,7 +376,6 @@ static void pc_q35_7_2_machine_options(MachineClass *m)
     pc_q35_machine_options(m);
     m->alias = "q35";
     pcmc->default_cpu_version = 1;
-    pcmc->legacy_no_rng_seed = true;
 }
 
 DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
@@ -384,8 +383,10 @@ DEFINE_Q35_MACHINE(v7_2, "pc-q35-7.2", NULL,
 
 static void pc_q35_7_1_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_7_2_machine_options(m);
     m->alias = NULL;
+    pcmc->legacy_no_rng_seed = true;
     compat_props_add(m->compat_props, hw_compat_7_1, hw_compat_7_1_len);
     compat_props_add(m->compat_props, pc_compat_7_1, pc_compat_7_1_len);
 }
-- 
2.37.3

Re: [PATCH v3 2/2] x86: re-enable rng seeding via setup_data

[Gerd Hoffmann]

On Tue, Sep 06, 2022 at 01:27:20PM +0200, Jason A. Donenfeld wrote:
> This reverts 3824e25db1 ("x86: disable rng seeding via setup_data"), but
> for 7.2 rather than 7.1, now that modifying setup_data is safe to do.
> 
> 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>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

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

[Ard Biesheuvel]

(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.

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

[Daniel P. Berrangé]

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

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

[Laszlo Ersek]

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

Laszlo

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

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

[Ard Biesheuvel]

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.

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

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

[Gerd Hoffmann via]

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>

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

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

[Jason A. Donenfeld]

On Tue, Sep 06, 2022 at 12:46:24PM +0200, Gerd Hoffmann 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>
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Thanks for the ack. Just FYI, we should probably wait until Ard sends
the kernel side of things up to Linus before committing this to QEMU.

Jason

Re: [PATCH] x86: only modify setup_data if the boot protocol indicates safety

[Jason A. Donenfeld]

Hi Gerd,

On Mon, Sep 5, 2022 at 10:40 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Sun, Sep 04, 2022 at 06:50:58PM +0200, Jason A. Donenfeld wrote:
> > This reverts 3824e25db1 ("x86: disable rng seeding via setup_data"), and
> > then makes the use of setup_data safe. It does so by checking the boot
> > protocol version. If it's sufficient, then it means EFI boots won't
> > crash. While we're at it, gate this on SEV too.
>
> > @@ -463,6 +462,7 @@ static void pc_i440fx_7_0_machine_options(MachineClass *m)
>
> > +    pcmc->legacy_no_rng_seed = true;
>
> This needs go into the pc_i440fx_7_1_machine_options function, otherwise
> legacy_no_rng_seed gets flipped from false to true for 7.1 machine types
> which breaks compatibility.
>
> > @@ -398,6 +397,7 @@ static void pc_q35_7_0_machine_options(MachineClass *m)
>
> > +    pcmc->legacy_no_rng_seed = true;
>
> Same here.

Oh. Okay so a "straight" revert won't do the trick, since this is (I
guess?) intended for 7.2 rather than 7.1. Makes sense; will do for v2.

>
> > --- a/hw/i386/x86.c
> > +++ b/hw/i386/x86.c
> > @@ -1088,8 +1088,15 @@ void x86_load_linux(X86MachineState *x86ms,
> >          qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
> >      }
> >
> > -    /* Offset 0x250 is a pointer to the first setup_data link. */
> > -    stq_p(header + 0x250, first_setup_data);
> > +    /*
> > +     * Only modify the header if doing so won't crash EFI boot, which is the
> > +     * case only for newer boot protocols, and don't do so either if SEV is
> > +     * enabled.
> > +     */
> > +    if (protocol >= 0x210 && !sev_enabled()) {
> > +        /* Offset 0x250 is a pointer to the first setup_data link. */
> > +        stq_p(header + 0x250, first_setup_data);
> > +    }
>
> This should better go into a separate patch.

Alright.

Jason

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

[Jason A. Donenfeld]

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>
---
 hw/i386/x86.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 050eedc0c8..fddc20df03 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1088,8 +1088,15 @@ void x86_load_linux(X86MachineState *x86ms,
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
     }
 
-    /* Offset 0x250 is a pointer to the first setup_data link. */
-    stq_p(header + 0x250, first_setup_data);
+    /*
+     * Only modify the header if doing so won't crash EFI boot, which is the
+     * case only for newer boot protocols, and don't do so either if SEV is
+     * enabled.
+     */
+    if (protocol >= 0x210 && !sev_enabled()) {
+        /* Offset 0x250 is a pointer to the first setup_data link. */
+        stq_p(header + 0x250, first_setup_data);
+    }
 
     /*
      * If we're starting an encrypted VM, it will be OVMF based, which uses the
-- 
2.37.3