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

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

[Jason A. Donenfeld]

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.

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 |  2 +-
 hw/i386/pc_q35.c  |  2 +-
 hw/i386/x86.c     | 11 +++++++++--
 4 files changed, 12 insertions(+), 5 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..ed7007672b 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,
@@ -463,6 +462,7 @@ static void pc_i440fx_7_0_machine_options(MachineClass *m)
     pc_i440fx_7_1_machine_options(m);
     m->alias = NULL;
     m->is_default = false;
+    pcmc->legacy_no_rng_seed = true;
     pcmc->enforce_amd_1tb_hole = false;
     compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 53eda50e81..3ffb40f8f0 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,
@@ -398,6 +397,7 @@ static void pc_q35_7_0_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_7_1_machine_options(m);
     m->alias = NULL;
+    pcmc->legacy_no_rng_seed = true;
     pcmc->enforce_amd_1tb_hole = false;
     compat_props_add(m->compat_props, hw_compat_7_0, hw_compat_7_0_len);
     compat_props_add(m->compat_props, pc_compat_7_0, pc_compat_7_0_len);
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] x86: only modify setup_data if the boot protocol indicates safety

[Jason A. Donenfeld]

FYI, this patch depends on this one in the kernel:
https://lore.kernel.org/linux-efi/20220904165321.1140894-1-Jason@zx2c4.com/

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

[Gerd Hoffmann]

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.

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

take care,
  Gerd

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

[PATCH v2 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] x86: only modify setup_data if the boot protocol indicates safety

[Gerd Hoffmann]

On Tue, Sep 06, 2022 at 12:27:25PM +0200, Jason A. Donenfeld wrote:
> 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.
> 
> 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.

Exactly.  7.1 is release and thus set in stone.  So we'll go flip the
switch for 7.2 because the feature missed the 7.1 boat.

take care,
  Gerd