Re: [PATCH] pc: add property for Linux setup_data seed

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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