[PULL,7/9] hw/guest-loader: pass random seed to fdt

From: "Jason A. Donenfeld" <Jason@zx2c4.com>

If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
initialize early. Set this using the usual guest random number
generation function. This FDT node is part of the DT specification.

Cc: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Message-Id: <20220719121559.135355-1-Jason@zx2c4.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/guest-loader.c | 5 +++++
 1 file changed, 5 insertions(+)

Paolo Bonzini <pbonzini@redhat.com> writes:

> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
>
> If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
> initialize early. Set this using the usual guest random number
> generation function. This FDT node is part of the DT specification.
>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Message-Id: <20220719121559.135355-1-Jason@zx2c4.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/guest-loader.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
> index 391c875a29..4f8572693c 100644
> --- a/hw/core/guest-loader.c
> +++ b/hw/core/guest-loader.c
> @@ -31,6 +31,7 @@
>  #include "hw/qdev-properties.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include "qemu/guest-random.h"
>  #include "guest-loader.h"
>  #include "sysemu/device_tree.h"
>  #include "hw/boards.h"
> @@ -46,6 +47,7 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
>      g_autofree char *node = g_strdup_printf("/chosen/module@0x%08" PRIx64,
>                                              s->addr);
>      uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
> +    uint8_t rng_seed[32];
>  
>      if (!fdt) {
>          error_setg(errp, "Cannot modify FDT fields if the machine has none");
> @@ -55,6 +57,9 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
>      qemu_fdt_add_subnode(fdt, node);
>      qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
>  
> +    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> +    qemu_fdt_setprop(fdt, node, "rng-seed", rng_seed, sizeof(rng_seed));
> +

Why are we inserting this here? The guest-loader is only building on
what the machine type has already constructed which in the case of -M
virt for riscv and ARM already has code for this.

>      if (s->kernel) {
>          const char *compat[2] = { "multiboot,module", "multiboot,kernel" };
>          if (qemu_fdt_setprop_string_array(fdt, node, "compatible",

Hi Alex,

On Thu, Jul 21, 2022 at 08:36:10PM +0100, Alex Bennée wrote:
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> >
> > If the FDT contains /chosen/rng-seed, then the Linux RNG will use it to
> > initialize early. Set this using the usual guest random number
> > generation function. This FDT node is part of the DT specification.
> >
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > Message-Id: <20220719121559.135355-1-Jason@zx2c4.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  hw/core/guest-loader.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/core/guest-loader.c b/hw/core/guest-loader.c
> > index 391c875a29..4f8572693c 100644
> > --- a/hw/core/guest-loader.c
> > +++ b/hw/core/guest-loader.c
> > @@ -31,6 +31,7 @@
> >  #include "hw/qdev-properties.h"
> >  #include "qapi/error.h"
> >  #include "qemu/module.h"
> > +#include "qemu/guest-random.h"
> >  #include "guest-loader.h"
> >  #include "sysemu/device_tree.h"
> >  #include "hw/boards.h"
> > @@ -46,6 +47,7 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
> >      g_autofree char *node = g_strdup_printf("/chosen/module@0x%08" PRIx64,
> >                                              s->addr);
> >      uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
> > +    uint8_t rng_seed[32];
> >  
> >      if (!fdt) {
> >          error_setg(errp, "Cannot modify FDT fields if the machine has none");
> > @@ -55,6 +57,9 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
> >      qemu_fdt_add_subnode(fdt, node);
> >      qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
> >  
> > +    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
> > +    qemu_fdt_setprop(fdt, node, "rng-seed", rng_seed, sizeof(rng_seed));
> > +
> 
> Why are we inserting this here? The guest-loader is only building on
> what the machine type has already constructed which in the case of -M
> virt for riscv and ARM already has code for this.

Wish you would have replied to the list patch before Paolo queued it.

I don't know this mechanism well but I assumed it would pass a unique
seed to each chained loader. Let me know if I'm misunderstanding the
purpose; I have no qualms about dropping this patch.

Jason

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> Hi Alex,
>
> On Thu, Jul 21, 2022 at 08:36:10PM +0100, Alex Bennée wrote:
>> 
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
<snip>
>> >      uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
>> > +    uint8_t rng_seed[32];
>> >  
>> >      if (!fdt) {
>> >          error_setg(errp, "Cannot modify FDT fields if the machine has none");
>> > @@ -55,6 +57,9 @@ static void loader_insert_platform_data(GuestLoaderState *s, int size,
>> >      qemu_fdt_add_subnode(fdt, node);
>> >      qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
>> >  
>> > +    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
>> > +    qemu_fdt_setprop(fdt, node, "rng-seed", rng_seed, sizeof(rng_seed));
>> > +
>> 
>> Why are we inserting this here? The guest-loader is only building on
>> what the machine type has already constructed which in the case of -M
>> virt for riscv and ARM already has code for this.
>
> Wish you would have replied to the list patch before Paolo queued it.

I'm sorry if I didn't get to it in the *checks notes* 2 days since it
was posted. I've been on holiday.

> I don't know this mechanism well but I assumed it would pass a unique
> seed to each chained loader. Let me know if I'm misunderstanding the
> purpose; I have no qualms about dropping this patch.

All the guest-loader does is add the information about where in memory a
guest and/or it's initrd have been placed in memory to the DTB. It's
entirely up to the initial booted code (usually a hypervisor in this
case) to decide what gets passed up the chain to any subsequent guests.

Hey Alex,

On Fri, Jul 22, 2022 at 10:45:19AM +0100, Alex Bennée wrote:
> All the guest-loader does is add the information about where in memory a
> guest and/or it's initrd have been placed in memory to the DTB. It's
> entirely up to the initial booted code (usually a hypervisor in this
> case) to decide what gets passed up the chain to any subsequent guests.

I think that's also my understanding, but let me tell you what I was
thinking with regards to rng-seed there, and you can tell me if I'm way
off.

The guest-loader puts in memory various loaders in a multistage boot.
Let's call it stage0, stage1, stage2, and finally the kernel. Normally,
rng-seed is only given to one of these stages. That stage may or may not
pass it to the next one, and it most probably does not. And why should
it? The host is in a better position to generate these seeds, rather
than adding finicky and fragile crypto ratcheting code into each stage
bootloader. So, instead, QEMU can just give each stage its own seed, for
it to do whatever with. This way, if stage1 does nothing, at least
there's a fresh unused one available for the kernel when it finally gets
there.

Does what I describe correspond at all with the use of guest-loader? If
so, maybe this patch should stay? If not, discard it as rubbish.

Jason

On 7/21/22 22:20, Jason A. Donenfeld wrote:
>> Why are we inserting this here? The guest-loader is only building on
>> what the machine type has already constructed which in the case of -M
>> virt for riscv and ARM already has code for this.
> 
> Wish you would have replied to the list patch before Paolo queued it.

Come on.

You posted a couple patches for this work 1 week before soft freeze 
(which is when maintainer trees should be ready for merge), so that some 
platforms get the support and some don't depending on how ready they 
are for the freeze itself.

Then you post the rest of the implementation on the day of the freeze. 
This patch has a pretty bad commit message too because any discussion on 
boot loader chaining belonged there.

Your own timing was completely off, and the right thing to do would have 
been to post a single series for all machines.  This way, even if the 
patches were to go via individual trees, maintainers could coordinate on 
which version to include, on how to handle migration, and so on.

Imagine doing the same thing for Linux, you'd be either ignored until 
the merge window ends, or alternatively shouted at.  Ignoring patches 
sent so close the soft freeze was my first instinct and it would have 
been the right call, except that in the meanwhile some architecture had 
their patches merged and here we are.

If anything _I_ have to apologize to Alex for picking up the patch in 
his stead, and for bending the soft freeze rules in an attempt to avoid 
having half-assed support where some architectures export the seed and 
some don't.  But you really have no standing to complain to him about 
not replying timely.

Thanks,

Paolo

Hi Paolo,

On Fri, Jul 22, 2022 at 02:04:45PM +0200, Paolo Bonzini wrote:
> On 7/21/22 22:20, Jason A. Donenfeld wrote:
> >> Why are we inserting this here? The guest-loader is only building on
> >> what the machine type has already constructed which in the case of -M
> >> virt for riscv and ARM already has code for this.
> > 
> > Wish you would have replied to the list patch before Paolo queued it.
> 
> Come on.
> 
> You posted a couple patches for this work 1 week before soft freeze 
> (which is when maintainer trees should be ready for merge), so that some 
> platforms get the support and some don't depending on how ready they 
> are for the freeze itself.
> 
> Then you post the rest of the implementation on the day of the freeze. 
> This patch has a pretty bad commit message too because any discussion on 
> boot loader chaining belonged there.
> 
> Your own timing was completely off, and the right thing to do would have 
> been to post a single series for all machines.  This way, even if the 
> patches were to go via individual trees, maintainers could coordinate on 
> which version to include, on how to handle migration, and so on.
> 
> Imagine doing the same thing for Linux, you'd be either ignored until 
> the merge window ends, or alternatively shouted at.  Ignoring patches 
> sent so close the soft freeze was my first instinct and it would have 
> been the right call, except that in the meanwhile some architecture had 
> their patches merged and here we are.
> 
> If anything _I_ have to apologize to Alex for picking up the patch in 
> his stead, and for bending the soft freeze rules in an attempt to avoid 
> having half-assed support where some architectures export the seed and 
> some don't.  But you really have no standing to complain to him about 
> not replying timely.

Please simmer down and quit the inane drama.

I don't have any qualms about Alex not replying in the two days before
you sent this pull. What I wish is that this was discussed on the list
before the pull so that we're now not in this awkward situation of
patch review inside of a pull. I don't know the procedures on what
happens now with that. Will this get pulled and now we have to revert?
Do you have to roll a new pull? I just have no idea, as this is all a
new thing for me. So my comment is more about the awkward state of
things than about some kind of failure from Alex. Obviously Alex is fine
here.

Your comments about my timing are also completely unjustified,
ridiculous, and actually a tad bit offensive. For the "high profile"
archs that I really wanted to hit 7.1, I started sending in DTB patches
a good deal of time ago. The only big arch I really wanted to hit 7.1
that wasn't queued up was the i386 patch, which I first posted in June.
Anyway, after it became clear that the i386 work was finally going to be
picked up, I breathed easy and decided to send in patches for the
remaining archs, to be picked up whenever. It was *your* decision that
all the DTB archs get in at the same time, hence picking this up; I had
no particular feelings on it, particularly as I don't know how to test
those remaining architectures like I did with the others. Anyway,
timing-wise, in my own planning, I handled risc-v, or1k, ppc, arm, i386,
and m68k well in advance and have been itching every single day since
posting those patches for them to be queued up somewhere.

So I really find your whole email just obnoxious and unnecessary. I've
been spending time trying to get the rng-seed stuff working on QEMU.
It's been a bit of a learning curve trying to figure out the QEMU
development model, and so I've miss-CC'd a few patches here and there.
But I've definitely tried to get an important subset of those patches in
in a timely manner. As a maintainer, you're definitely having the effect
of turning me off of the project rather than trying to acquaint me with
norms or be helpful.

Please, quit the drama. Enough of this stuff.

Jason

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> Hey Alex,
>
> On Fri, Jul 22, 2022 at 10:45:19AM +0100, Alex Bennée wrote:
>> All the guest-loader does is add the information about where in memory a
>> guest and/or it's initrd have been placed in memory to the DTB. It's
>> entirely up to the initial booted code (usually a hypervisor in this
>> case) to decide what gets passed up the chain to any subsequent guests.
>
> I think that's also my understanding, but let me tell you what I was
> thinking with regards to rng-seed there, and you can tell me if I'm way
> off.
>
> The guest-loader puts in memory various loaders in a multistage boot.
> Let's call it stage0, stage1, stage2, and finally the kernel. Normally,
> rng-seed is only given to one of these stages. That stage may or may not
> pass it to the next one, and it most probably does not. And why should
> it? The host is in a better position to generate these seeds, rather
> than adding finicky and fragile crypto ratcheting code into each stage
> bootloader. So, instead, QEMU can just give each stage its own seed, for
> it to do whatever with. This way, if stage1 does nothing, at least
> there's a fresh unused one available for the kernel when it finally gets
> there.

That sounds suspiciously like inventing a new ABI between QEMU and
guests which we generally try to avoid. The DTB exposed to the first
stage may never be made visible to the following stages or more likely a
sanitised version is prepared by the previous stage. Generally QEMU just
tries to get the emulation right so the firmware/software can get on
with it's thing. Indeed the dynamic DTB for -M virt and friends is an
oddity compared to most of the other machine types which assume the user
has a valid DTB.

Either way given how close to release we are I'd rather drop this patch.

> Does what I describe correspond at all with the use of guest-loader? If
> so, maybe this patch should stay? If not, discard it as rubbish.

The original intent of the guest-loader was to make testing of
hypervisors easier because the alternative is getting a multi-stage boot
chain of firmware, boot-loaders and distro specific integration working
which can be quite opaque to debug (c.f. why -kernel/-initrd exist and
not everyone boots via -bios/-pflash).

>
> Jason

Ok I will resend the pull request. Apologies for overstepping.

Paolo

Il ven 22 lug 2022, 16:37 Alex Bennée <alex.bennee@linaro.org> ha scritto:

>
> "Jason A. Donenfeld" <Jason@zx2c4.com> writes:
>
> > Hey Alex,
> >
> > On Fri, Jul 22, 2022 at 10:45:19AM +0100, Alex Bennée wrote:
> >> All the guest-loader does is add the information about where in memory a
> >> guest and/or it's initrd have been placed in memory to the DTB. It's
> >> entirely up to the initial booted code (usually a hypervisor in this
> >> case) to decide what gets passed up the chain to any subsequent guests.
> >
> > I think that's also my understanding, but let me tell you what I was
> > thinking with regards to rng-seed there, and you can tell me if I'm way
> > off.
> >
> > The guest-loader puts in memory various loaders in a multistage boot.
> > Let's call it stage0, stage1, stage2, and finally the kernel. Normally,
> > rng-seed is only given to one of these stages. That stage may or may not
> > pass it to the next one, and it most probably does not. And why should
> > it? The host is in a better position to generate these seeds, rather
> > than adding finicky and fragile crypto ratcheting code into each stage
> > bootloader. So, instead, QEMU can just give each stage its own seed, for
> > it to do whatever with. This way, if stage1 does nothing, at least
> > there's a fresh unused one available for the kernel when it finally gets
> > there.
>
> That sounds suspiciously like inventing a new ABI between QEMU and
> guests which we generally try to avoid. The DTB exposed to the first
> stage may never be made visible to the following stages or more likely a
> sanitised version is prepared by the previous stage. Generally QEMU just
> tries to get the emulation right so the firmware/software can get on
> with it's thing. Indeed the dynamic DTB for -M virt and friends is an
> oddity compared to most of the other machine types which assume the user
> has a valid DTB.
>
> Either way given how close to release we are I'd rather drop this patch.
>
> > Does what I describe correspond at all with the use of guest-loader? If
> > so, maybe this patch should stay? If not, discard it as rubbish.
>
> The original intent of the guest-loader was to make testing of
> hypervisors easier because the alternative is getting a multi-stage boot
> chain of firmware, boot-loaders and distro specific integration working
> which can be quite opaque to debug (c.f. why -kernel/-initrd exist and
> not everyone boots via -bios/-pflash).
>
> >
> > Jason
>
>
> --
> Alex Bennée
>
>

Hi Alex,

On Fri, Jul 22, 2022 at 4:37 PM Alex Bennée <alex.bennee@linaro.org> wrote:
> That sounds suspiciously like inventing a new ABI between QEMU and
> guests which we generally try to avoid.

Well the ABI is just the "rng-seed" param which is part of the DT
spec. But I can understand why you might find this use a bit "too
creative". So no qualms about dropping it.

Jason