hw/i386/microvm.c | 2 +- hw/i386/pc.c | 5 +++-- hw/i386/pc_piix.c | 2 -- hw/i386/pc_q35.c | 2 -- hw/i386/x86.c | 31 +++++++++++++++++++++++++++---- include/hw/i386/pc.h | 3 --- include/hw/i386/x86.h | 5 +++-- 7 files changed, 34 insertions(+), 16 deletions(-)
Using a property makes it possible to use the normal compat property
mechanism instead of ad hoc code; it avoids parameter proliferation
in x86_load_linux; and allows shipping the code even if it is
disabled by default.
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/i386/microvm.c | 2 +-
hw/i386/pc.c | 5 +++--
hw/i386/pc_piix.c | 2 --
hw/i386/pc_q35.c | 2 --
hw/i386/x86.c | 31 +++++++++++++++++++++++++++----
include/hw/i386/pc.h | 3 ---
include/hw/i386/x86.h | 5 +++--
7 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 7fe8cce03e..dc929727dc 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, false);
+ x86_load_linux(x86ms, fw_cfg, 0, true);
}
if (mms->option_roms) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7280c02ce3..1a77f5f0f0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -112,6 +112,7 @@ const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0);
GlobalProperty pc_compat_6_2[] = {
{ "virtio-mem", "unplugged-inaccessible", "off" },
+ { TYPE_X86_MACHINE, "linuxboot-seed", "off" },
};
const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2);
@@ -796,7 +797,7 @@ void xen_load_linux(PCMachineState *pcms)
rom_set_fw(fw_cfg);
x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
- pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
+ pcmc->pvh_enabled);
for (i = 0; i < nb_option_roms; i++) {
assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
!strcmp(option_rom[i].name, "linuxboot_dma.bin") ||
@@ -1118,7 +1119,7 @@ void pc_memory_init(PCMachineState *pcms,
if (linux_boot) {
x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
- pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
+ pcmc->pvh_enabled);
}
for (i = 0; i < nb_option_roms; i++) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a5c65c1c35..00c21f6e4d 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -446,11 +446,9 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
static void pc_i440fx_7_0_machine_options(MachineClass *m)
{
- PCMachineClass *pcmc = PC_MACHINE_CLASS(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 3a35193ff7..5bcf100b35 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -383,10 +383,8 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
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..3fbab258a9 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -767,8 +767,7 @@ static bool load_elfboot(const char *kernel_filename,
void x86_load_linux(X86MachineState *x86ms,
FWCfgState *fw_cfg,
int acpi_data_size,
- bool pvh_enabled,
- bool legacy_no_rng_seed)
+ bool pvh_enabled)
{
bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
uint16_t protocol;
@@ -786,7 +785,6 @@ void x86_load_linux(X86MachineState *x86ms,
const char *dtb_filename = machine->dtb;
const char *kernel_cmdline = machine->kernel_cmdline;
SevKernelLoaderContext sev_load_ctx = {};
- enum { RNG_SEED_LENGTH = 32 };
/* Align to 16 bytes as a paranoia measure */
cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -1076,7 +1074,8 @@ void x86_load_linux(X86MachineState *x86ms,
load_image_size(dtb_filename, setup_data->data, dtb_size);
}
- if (!legacy_no_rng_seed) {
+ if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
+ (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
kernel = g_realloc(kernel, kernel_size);
@@ -1237,6 +1236,23 @@ static void x86_machine_set_smm(Object *obj, Visitor *v, const char *name,
visit_type_OnOffAuto(v, name, &x86ms->smm, errp);
}
+static void x86_machine_get_linuxboot_seed(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ X86MachineState *x86ms = X86_MACHINE(obj);
+ OnOffAuto linuxboot_seed = x86ms->linuxboot_seed;
+
+ visit_type_OnOffAuto(v, name, &linuxboot_seed, errp);
+}
+
+static void x86_machine_set_linuxboot_seed(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ X86MachineState *x86ms = X86_MACHINE(obj);
+
+ visit_type_OnOffAuto(v, name, &x86ms->linuxboot_seed, errp);
+}
+
bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms)
{
if (x86ms->acpi == ON_OFF_AUTO_OFF) {
@@ -1387,6 +1403,7 @@ static void x86_machine_initfn(Object *obj)
x86ms->acpi = ON_OFF_AUTO_AUTO;
x86ms->pit = ON_OFF_AUTO_AUTO;
x86ms->pic = ON_OFF_AUTO_AUTO;
+ x86ms->linuxboot_seed = ON_OFF_AUTO_AUTO;
x86ms->pci_irq_mask = ACPI_BUILD_PCI_IRQS;
x86ms->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
x86ms->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
@@ -1426,6 +1443,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data)
object_class_property_set_description(oc, X86_MACHINE_PIT,
"Enable i8254 PIT");
+ object_class_property_add(oc, X86_MACHINE_LINUXBOOT_SEED, "OnOffAuto",
+ x86_machine_get_linuxboot_seed, x86_machine_set_linuxboot_seed,
+ NULL, NULL);
+ object_class_property_set_description(oc, X86_MACHINE_LINUXBOOT_SEED,
+ "Pass random number seed to -kernel Linux image");
+
object_class_property_add(oc, X86_MACHINE_PIC, "OnOffAuto",
x86_machine_get_pic,
x86_machine_set_pic,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8435733bd6..9cc3f5d338 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -128,9 +128,6 @@ struct PCMachineClass {
/* create kvmclock device even when KVM PV features are not exposed */
bool kvmclock_create_always;
-
- /* skip passing an rng seed for legacy machines */
- bool legacy_no_rng_seed;
};
#define TYPE_PC_MACHINE "generic-pc-machine"
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 62fa5774f8..a27f4adbf8 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -70,6 +70,7 @@ struct X86MachineState {
OnOffAuto acpi;
OnOffAuto pit;
OnOffAuto pic;
+ OnOffAuto linuxboot_seed;
char *oem_id;
char *oem_table_id;
@@ -94,6 +95,7 @@ struct X86MachineState {
#define X86_MACHINE_OEM_ID "x-oem-id"
#define X86_MACHINE_OEM_TABLE_ID "x-oem-table-id"
#define X86_MACHINE_BUS_LOCK_RATELIMIT "bus-lock-ratelimit"
+#define X86_MACHINE_LINUXBOOT_SEED "linuxboot-seed"
#define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86")
OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE)
@@ -126,8 +128,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
void x86_load_linux(X86MachineState *x86ms,
FWCfgState *fw_cfg,
int acpi_data_size,
- bool pvh_enabled,
- bool legacy_no_rng_seed);
+ bool pvh_enabled);
bool x86_machine_is_smm_enabled(const X86MachineState *x86ms);
bool x86_machine_is_acpi_enabled(const X86MachineState *x86ms);
--
2.37.1On Thu, Aug 04, 2022 at 03:13:20PM +0200, Paolo Bonzini wrote:
> Using a property makes it possible to use the normal compat property
> mechanism instead of ad hoc code; it avoids parameter proliferation
> in x86_load_linux; and allows shipping the code even if it is
> disabled by default.
>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/i386/microvm.c | 2 +-
> hw/i386/pc.c | 5 +++--
> hw/i386/pc_piix.c | 2 --
> hw/i386/pc_q35.c | 2 --
> hw/i386/x86.c | 31 +++++++++++++++++++++++++++----
> include/hw/i386/pc.h | 3 ---
> include/hw/i386/x86.h | 5 +++--
> 7 files changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index 7fe8cce03e..dc929727dc 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, false);
> + x86_load_linux(x86ms, fw_cfg, 0, true);
> }
>
> if (mms->option_roms) {
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7280c02ce3..1a77f5f0f0 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -112,6 +112,7 @@ const size_t pc_compat_7_0_len = G_N_ELEMENTS(pc_compat_7_0);
>
> GlobalProperty pc_compat_6_2[] = {
> { "virtio-mem", "unplugged-inaccessible", "off" },
> + { TYPE_X86_MACHINE, "linuxboot-seed", "off" },
> };
> const size_t pc_compat_6_2_len = G_N_ELEMENTS(pc_compat_6_2);
>
> @@ -796,7 +797,7 @@ void xen_load_linux(PCMachineState *pcms)
> rom_set_fw(fw_cfg);
>
> x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> - pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
> + pcmc->pvh_enabled);
> for (i = 0; i < nb_option_roms; i++) {
> assert(!strcmp(option_rom[i].name, "linuxboot.bin") ||
> !strcmp(option_rom[i].name, "linuxboot_dma.bin") ||
> @@ -1118,7 +1119,7 @@ void pc_memory_init(PCMachineState *pcms,
>
> if (linux_boot) {
> x86_load_linux(x86ms, fw_cfg, pcmc->acpi_data_size,
> - pcmc->pvh_enabled, pcmc->legacy_no_rng_seed);
> + pcmc->pvh_enabled);
> }
>
> for (i = 0; i < nb_option_roms; i++) {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a5c65c1c35..00c21f6e4d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -446,11 +446,9 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
>
> static void pc_i440fx_7_0_machine_options(MachineClass *m)
> {
> - PCMachineClass *pcmc = PC_MACHINE_CLASS(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 3a35193ff7..5bcf100b35 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -383,10 +383,8 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
>
> 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..3fbab258a9 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -767,8 +767,7 @@ static bool load_elfboot(const char *kernel_filename,
> void x86_load_linux(X86MachineState *x86ms,
> FWCfgState *fw_cfg,
> int acpi_data_size,
> - bool pvh_enabled,
> - bool legacy_no_rng_seed)
> + bool pvh_enabled)
> {
> bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
> uint16_t protocol;
> @@ -786,7 +785,6 @@ void x86_load_linux(X86MachineState *x86ms,
> const char *dtb_filename = machine->dtb;
> const char *kernel_cmdline = machine->kernel_cmdline;
> SevKernelLoaderContext sev_load_ctx = {};
> - enum { RNG_SEED_LENGTH = 32 };
>
> /* Align to 16 bytes as a paranoia measure */
> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -1076,7 +1074,8 @@ void x86_load_linux(X86MachineState *x86ms,
> load_image_size(dtb_filename, setup_data->data, dtb_size);
> }
>
> - if (!legacy_no_rng_seed) {
> + if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
> + (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
> setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
> kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
> kernel = g_realloc(kernel, kernel_size);
This fails to build
../hw/i386/x86.c: In function ‘x86_load_linux’:
../hw/i386/x86.c:1078:10: error: ‘data’ undeclared (first use in this function)
1078 | (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
| ^~~~
../hw/i386/x86.c:1078:10: note: each undeclared identifier is reported only once for each function it appears in
../hw/i386/x86.c:1080:71: error: ‘RNG_SEED_LENGTH’ undeclared (first use in this function)
1080 | kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
| ^~~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.
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 :|
On Thu, Aug 04, 2022 at 03:13:20PM +0200, Paolo Bonzini wrote: > Using a property makes it possible to use the normal compat property > mechanism instead of ad hoc code; it avoids parameter proliferation > in x86_load_linux; and allows shipping the code even if it is > disabled by default. Strong NACK from me here. If this kind of thing is off by default, it's as good as useless. Indeed it shouldn't even be a knob at all. Don't do this. Rather, let's fix the bug. The code as-is -- going back to the 2016 DTB addition -- is problematic and needs to be fixed. So let's fix that. Trying to cover up the problem with a default-off knob just ensures this stuff will never be made to work right. Please do not commit this patch. Rather, let's see what a few round of review can do over on the fix I posted. We're still in rc0. No need to jump to something terrible like this. Jason
On Thu, Aug 04, 2022 at 03:20:59PM +0200, Jason A. Donenfeld wrote: > On Thu, Aug 04, 2022 at 03:13:20PM +0200, Paolo Bonzini wrote: > > Using a property makes it possible to use the normal compat property > > mechanism instead of ad hoc code; it avoids parameter proliferation > > in x86_load_linux; and allows shipping the code even if it is > > disabled by default. > > Strong NACK from me here. > > If this kind of thing is off by default, it's as good as useless. Indeed > it shouldn't even be a knob at all. Don't do this. You're misunderstanding the patch. This remains on by default for the 7.1 machine type. The patch is merely exposing a knob so that users can override the built-in default if they need to. Imagine if we had shipped this existing code before today's bugs were discovered. The knob proposed her would allow users to turn off the broken pieces. This is a good thing. > Rather, let's fix the bug. The code as-is -- going back to the 2016 DTB > addition -- is problematic and needs to be fixed. So let's fix that. > Trying to cover up the problem with a default-off knob just ensures this > stuff will never be made to work right. It isn't covering up the problem, just providing a workaround option, should another bug be discovered after release. We still need to fix current discussed problems of course. 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 :|
Hi Daniel,
On Thu, Aug 04, 2022 at 02:31:06PM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 04, 2022 at 03:20:59PM +0200, Jason A. Donenfeld wrote:
> > On Thu, Aug 04, 2022 at 03:13:20PM +0200, Paolo Bonzini wrote:
> > > Using a property makes it possible to use the normal compat property
> > > mechanism instead of ad hoc code; it avoids parameter proliferation
> > > in x86_load_linux; and allows shipping the code even if it is
> > > disabled by default.
> >
> > Strong NACK from me here.
> >
> > If this kind of thing is off by default, it's as good as useless. Indeed
> > it shouldn't even be a knob at all. Don't do this.
>
> You're misunderstanding the patch. This remains on by default for
> the 7.1 machine type.
Ahhh, I think you're right. Sorry for mis understanding. The "even if it
is disabled by default" of the commit message isn't quite true then,
right?
If I understand correctly, this is a yes/no/auto, which defaults to
auto on newer machine types. And auto triggers if the kernel has a newer
boot header. Is that correct?
if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
(protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
I think it's working as described (after applying the below fixup to
this broken patch).
> The patch is merely exposing a knob so that users can override the
> built-in default if they need to. Imagine if we had shipped this
> existing code before today's bugs were discovered. The knob
> proposed her would allow users to turn off the broken pieces.
> This is a good thing.
I'm still not really keen on adding a knob for this. I understand ARM
has a knob for it for different reasons (better named "dtb-randomness").
If this knob thing is to live on here, maybe it should have
"-randomness" in the name also.
> > Rather, let's fix the bug. The code as-is -- going back to the 2016 DTB
> > addition -- is problematic and needs to be fixed. So let's fix that.
> > Trying to cover up the problem with a default-off knob just ensures this
> > stuff will never be made to work right.
>
> It isn't covering up the problem, just providing a workaround
> option, should another bug be discovered after release. We
> still need to fix current discussed problems of course.
Thanks for the explanation. I don't like adding a knob. But if it's on
by default for the default machine type, then that's a compromise I
could accept.
Jason
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 00c21f6e4d..074571bc03 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -446,6 +446,7 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
static void pc_i440fx_7_0_machine_options(MachineClass *m)
{
+ PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
pc_i440fx_7_1_machine_options(m);
m->alias = NULL;
m->is_default = false;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 5bcf100b35..f3aa4694a2 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -383,6 +383,7 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
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->enforce_amd_1tb_hole = false;
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 3fbab258a9..206ce6c547 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -785,6 +785,7 @@ void x86_load_linux(X86MachineState *x86ms,
const char *dtb_filename = machine->dtb;
const char *kernel_cmdline = machine->kernel_cmdline;
SevKernelLoaderContext sev_load_ctx = {};
+ enum { RNG_SEED_LENGTH = 32 };
/* Align to 16 bytes as a paranoia measure */
cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -1075,7 +1076,7 @@ void x86_load_linux(X86MachineState *x86ms,
}
if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
- (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
+ (protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
kernel = g_realloc(kernel, kernel_size);
On 8/4/22 16:31, Jason A. Donenfeld wrote: > I'm still not really keen on adding a knob for this. I understand ARM > has a knob for it for different reasons (better named "dtb-randomness"). > If this knob thing is to live on here, maybe it should have > "-randomness" in the name also. Ok, I just reused your variable name but linuxboot-randomness is fine by me too. >>> Rather, let's fix the bug. The code as-is -- going back to the 2016 DTB >>> addition -- is problematic and needs to be fixed. So let's fix that. >>> Trying to cover up the problem with a default-off knob just ensures this >>> stuff will never be made to work right. >> >> It isn't covering up the problem, just providing a workaround >> option, should another bug be discovered after release. We >> still need to fix current discussed problems of course. > > Thanks for the explanation. I don't like adding a knob. But if it's on > by default for the default machine type, then that's a compromise I > could accept. Yes, in fact this allows enabling the seed even for older machine types if everything goes fine. And if it doesn't, we only need a one-line patch to revert the feature, like Michael said. So it's a good thing to have either way. The patch was extracted out of my version from last month, but I didn't --amend the changes needed to make it compile (doh). I incorporated yours instead and I'll send v2. Paolo
On Thu, Aug 04, 2022 at 04:31:06PM +0200, Jason A. Donenfeld wrote:
> Hi Daniel,
>
> On Thu, Aug 04, 2022 at 02:31:06PM +0100, Daniel P. Berrangé wrote:
> > On Thu, Aug 04, 2022 at 03:20:59PM +0200, Jason A. Donenfeld wrote:
> > > On Thu, Aug 04, 2022 at 03:13:20PM +0200, Paolo Bonzini wrote:
> > > > Using a property makes it possible to use the normal compat property
> > > > mechanism instead of ad hoc code; it avoids parameter proliferation
> > > > in x86_load_linux; and allows shipping the code even if it is
> > > > disabled by default.
> > >
> > > Strong NACK from me here.
> > >
> > > If this kind of thing is off by default, it's as good as useless. Indeed
> > > it shouldn't even be a knob at all. Don't do this.
> >
> > You're misunderstanding the patch. This remains on by default for
> > the 7.1 machine type.
>
> Ahhh, I think you're right. Sorry for mis understanding. The "even if it
> is disabled by default" of the commit message isn't quite true then,
> right?
I think it refers to the fact that we do not have a fix yet,
and if we don't get one by release we might flip the default,
but the feature will still be usable.
> If I understand correctly, this is a yes/no/auto, which defaults to
> auto on newer machine types. And auto triggers if the kernel has a newer
> boot header. Is that correct?
>
> if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
> (protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
>
> I think it's working as described (after applying the below fixup to
> this broken patch).
>
> > The patch is merely exposing a knob so that users can override the
> > built-in default if they need to. Imagine if we had shipped this
> > existing code before today's bugs were discovered. The knob
> > proposed her would allow users to turn off the broken pieces.
> > This is a good thing.
>
> I'm still not really keen on adding a knob for this. I understand ARM
> has a knob for it for different reasons (better named "dtb-randomness").
> If this knob thing is to live on here, maybe it should have
> "-randomness" in the name also.
>
> > > Rather, let's fix the bug. The code as-is -- going back to the 2016 DTB
> > > addition -- is problematic and needs to be fixed. So let's fix that.
> > > Trying to cover up the problem with a default-off knob just ensures this
> > > stuff will never be made to work right.
> >
> > It isn't covering up the problem, just providing a workaround
> > option, should another bug be discovered after release. We
> > still need to fix current discussed problems of course.
>
> Thanks for the explanation. I don't like adding a knob. But if it's on
> by default for the default machine type, then that's a compromise I
> could accept.
>
> Jason
>
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 00c21f6e4d..074571bc03 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -446,6 +446,7 @@ DEFINE_I440FX_MACHINE(v7_1, "pc-i440fx-7.1", NULL,
>
> static void pc_i440fx_7_0_machine_options(MachineClass *m)
> {
> + PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> pc_i440fx_7_1_machine_options(m);
> m->alias = NULL;
> m->is_default = false;
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 5bcf100b35..f3aa4694a2 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -383,6 +383,7 @@ DEFINE_Q35_MACHINE(v7_1, "pc-q35-7.1", NULL,
>
> 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->enforce_amd_1tb_hole = false;
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 3fbab258a9..206ce6c547 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -785,6 +785,7 @@ void x86_load_linux(X86MachineState *x86ms,
> const char *dtb_filename = machine->dtb;
> const char *kernel_cmdline = machine->kernel_cmdline;
> SevKernelLoaderContext sev_load_ctx = {};
> + enum { RNG_SEED_LENGTH = 32 };
>
> /* Align to 16 bytes as a paranoia measure */
> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -1075,7 +1076,7 @@ void x86_load_linux(X86MachineState *x86ms,
> }
>
> if (x86ms->linuxboot_seed != ON_OFF_AUTO_OFF &&
> - (data.protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
> + (protocol >= 0x209 || x86ms->linuxboot_seed == ON_OFF_AUTO_ON)) {
> setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
> kernel_size = setup_data_offset + sizeof(struct setup_data) + RNG_SEED_LENGTH;
> kernel = g_realloc(kernel, kernel_size);
© 2016 - 2024 Red Hat, Inc.