| Message ID | 20220904165058.1140503-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: only modify setup_data if the boot protocol indicates safety | expand |
FYI, this patch depends on this one in the kernel: https://lore.kernel.org/linux-efi/20220904165321.1140894-1-Jason@zx2c4.com/
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
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
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
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
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(-)