[PATCH 1/2] m68k: process bootinfo records before saving them

[PATCH 1/2] m68k: process bootinfo records before saving them

[Jason A. Donenfeld]

The RNG seed boot record is memzeroed after processing, in order to
preserve forward secrecy. By saving the bootinfo for procfs prior to
that, forward secrecy is violated, since it becomes possible to recover
past states. So, save the bootinfo block only after first processing
them.

Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/m68k/kernel/setup_mm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
index e62fa8f2149b..7e7ef67cff8b 100644
--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -109,10 +109,9 @@ extern void paging_init(void);
 
 static void __init m68k_parse_bootinfo(const struct bi_record *record)
 {
+	const struct bi_record *first_record = record;
 	uint16_t tag;
 
-	save_bootinfo(record);
-
 	while ((tag = be16_to_cpu(record->tag)) != BI_LAST) {
 		int unknown = 0;
 		const void *data = record->data;
@@ -182,6 +181,8 @@ static void __init m68k_parse_bootinfo(const struct bi_record *record)
 		record = (struct bi_record *)((unsigned long)record + size);
 	}
 
+	save_bootinfo(first_record);
+
 	m68k_realnum_memory = m68k_num_memory;
 #ifdef CONFIG_SINGLE_MEMORY_CHUNK
 	if (m68k_num_memory > 1) {
-- 
2.37.3

[PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

Rather than rebooting into a system with no entropy, regenerate the RNG
seed before rebooting, so that the new system has a fresh seed.

Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/m68k/virt/config.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/m68k/virt/config.c b/arch/m68k/virt/config.c
index 4ab22946ff68..c41d2277de95 100644
--- a/arch/m68k/virt/config.c
+++ b/arch/m68k/virt/config.c
@@ -45,10 +45,18 @@ static void virt_halt(void)
 		;
 }
 
+static struct bi_record *rng_seed_record;
+
 static void virt_reset(void)
 {
 	void __iomem *base = (void __iomem *)virt_bi_data.ctrl.mmio;
 
+	if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
+		u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
+		get_random_bytes((u8 *)rng_seed_record->data + 2, len);
+		*(u16 *)rng_seed_record->data = len;
+	}
+
 	iowrite32be(CMD_RESET, base + VIRT_CTRL_REG_CMD);
 	local_irq_disable();
 	while (1)
@@ -101,6 +109,8 @@ int __init virt_parse_bootinfo(const struct bi_record *record)
 		 * length to prevent kexec from using it.
 		 */
 		memzero_explicit((void *)data, len + 2);
+		 /* Store a reference to be filled in on reboot. */
+		rng_seed_record = (void *)record;
 		break;
 	}
 	default:
-- 
2.37.3

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Geert Uytterhoeven]

Hi Jason,

On Wed, Sep 21, 2022 at 8:52 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Rather than rebooting into a system with no entropy, regenerate the RNG
> seed before rebooting, so that the new system has a fresh seed.
>
> Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Thanks for your patch!

> --- a/arch/m68k/virt/config.c
> +++ b/arch/m68k/virt/config.c
> @@ -45,10 +45,18 @@ static void virt_halt(void)
>                 ;
>  }
>
> +static struct bi_record *rng_seed_record;

This can be const...

> +
>  static void virt_reset(void)
>  {
>         void __iomem *base = (void __iomem *)virt_bi_data.ctrl.mmio;
>
> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> +               *(u16 *)rng_seed_record->data = len;

Wouldn't it be simpler to just use the existing length?

    if (rnd_seed_record) {
           u16 len = be16_to_cpup(data);
           get_random_bytes((u8 *)rng_seed_record->data + 2, len);
    }

However, I have my doubts this will actually work. Was this tested?
The bootinfo is passed from userspace, usually by reading
/proc/bootinfo, and adapting it where needed.
So I think you should implement this in kexec-tools instead.

> +       }
> +
>         iowrite32be(CMD_RESET, base + VIRT_CTRL_REG_CMD);
>         local_irq_disable();
>         while (1)
> @@ -101,6 +109,8 @@ int __init virt_parse_bootinfo(const struct bi_record *record)
>                  * length to prevent kexec from using it.
>                  */
>                 memzero_explicit((void *)data, len + 2);
> +                /* Store a reference to be filled in on reboot. */
> +               rng_seed_record = (void *)record;

... so this cast can be dropped.

>                 break;
>         }
>         default:

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

Hi Geert,

On Fri, Sep 23, 2022 at 1:30 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > +static struct bi_record *rng_seed_record;
>
> This can be const...
> >                 memzero_explicit((void *)data, len + 2);
> > +                /* Store a reference to be filled in on reboot. */
> > +               rng_seed_record = (void *)record;
>
> ... so this cast can be dropped.

Will do.

>
> > +
> >  static void virt_reset(void)
> >  {
> >         void __iomem *base = (void __iomem *)virt_bi_data.ctrl.mmio;
> >
> > +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> > +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> > +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> > +               *(u16 *)rng_seed_record->data = len;
>
> Wouldn't it be simpler to just use the existing length?
>
>     if (rnd_seed_record) {
>            u16 len = be16_to_cpup(data);
>            get_random_bytes((u8 *)rng_seed_record->data + 2, len);
>     }

No, that would not work. len is 0 there, since we zero out the bytes
after use for forward secrecy, and we zero out the length, so that we
don't wind up feeding it zeros.

>
> However, I have my doubts this will actually work. Was this tested?
> The bootinfo is passed from userspace, usually by reading
> /proc/bootinfo, and adapting it where needed.
> So I think you should implement this in kexec-tools instead.

Yes, this was tested. This is to handle the reboot case, just as the
commit subject says. Specifically, calling `reboot(RB_AUTOBOOT);`.

It does *not* handle kexec. For that, indeed, kexec-tools needs to be
augmented, but that's a separate patch that doesn't need to interact
with this one.

The way I tested this is by having my initramfs just call
`reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
its contents to the console. I checked that it was both present and
different every time.

I'll send a v2 with that const fix.

Jason

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Geert Uytterhoeven]

Hi Jason,

On Fri, Sep 23, 2022 at 1:53 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Fri, Sep 23, 2022 at 1:30 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > +static struct bi_record *rng_seed_record;
> >
> > This can be const...
> > >                 memzero_explicit((void *)data, len + 2);
> > > +                /* Store a reference to be filled in on reboot. */
> > > +               rng_seed_record = (void *)record;
> >
> > ... so this cast can be dropped.
>
> Will do.
>
> >
> > > +
> > >  static void virt_reset(void)
> > >  {
> > >         void __iomem *base = (void __iomem *)virt_bi_data.ctrl.mmio;
> > >
> > > +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> > > +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> > > +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> > > +               *(u16 *)rng_seed_record->data = len;

Storing the length should use the proper cpu_to_be16 accessor.

> > Wouldn't it be simpler to just use the existing length?
> >
> >     if (rnd_seed_record) {
> >            u16 len = be16_to_cpup(data);
> >            get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> >     }
>
> No, that would not work. len is 0 there, since we zero out the bytes
> after use for forward secrecy, and we zero out the length, so that we
> don't wind up feeding it zeros.

You're right. I misread the location of the "+ 2" in the clearing code.

> > However, I have my doubts this will actually work. Was this tested?
> > The bootinfo is passed from userspace, usually by reading
> > /proc/bootinfo, and adapting it where needed.
> > So I think you should implement this in kexec-tools instead.
>
> Yes, this was tested. This is to handle the reboot case, just as the
> commit subject says. Specifically, calling `reboot(RB_AUTOBOOT);`.

OK.

> It does *not* handle kexec. For that, indeed, kexec-tools needs to be
> augmented, but that's a separate patch that doesn't need to interact
> with this one.
>
> The way I tested this is by having my initramfs just call
> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> its contents to the console. I checked that it was both present and
> different every time.

Are you sure the new kernel did receive the same randomness as prepared
by get_random_bytes()? I would expect it to just reboot into qemu,
reload the kernel from disk, and recreate a new bootinfo from scratch,
including generating a new random seed.

> I'll send a v2 with that const fix.

OK, thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> > > > +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> > > > +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> > > > +               *(u16 *)rng_seed_record->data = len;
>
> Storing the length should use the proper cpu_to_be16 accessor.

Okay, I'll do that for v2.

(Simply out of curiosity, why? Isn't m68k always big endian and this
is arch/ code?)

> > The way I tested this is by having my initramfs just call
> > `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> > its contents to the console. I checked that it was both present and
> > different every time.
>
> Are you sure the new kernel did receive the same randomness as prepared
> by get_random_bytes()? I would expect it to just reboot into qemu,
> reload the kernel from disk, and recreate a new bootinfo from scratch,
> including generating a new random seed.

Yes I'm sure. Without this patch, the new kernel sees the zeroed state.

Jason

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Geert Uytterhoeven]

Hi Jason,

On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> > > > > +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> > > > > +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> > > > > +               *(u16 *)rng_seed_record->data = len;
> >
> > Storing the length should use the proper cpu_to_be16 accessor.
>
> Okay, I'll do that for v2.
>
> (Simply out of curiosity, why? Isn't m68k always big endian and this
> is arch/ code?)

Yes it is.  But virt_parse_bootinfo() below already uses the right
accessor.

BTW, I guess people thought the same about PowerPC?
Although I agree the probability of someone creating a little-endian
m68k clone in an FPGA or SkyWater project and trying to run Linux on
it quite low ;-)

> > > The way I tested this is by having my initramfs just call
> > > `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> > > its contents to the console. I checked that it was both present and
> > > different every time.
> >
> > Are you sure the new kernel did receive the same randomness as prepared
> > by get_random_bytes()? I would expect it to just reboot into qemu,
> > reload the kernel from disk, and recreate a new bootinfo from scratch,
> > including generating a new random seed.
>
> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.

That's interesting.  So QEMU preserves the old bootinfo, which is
AFAIK not guaranteed to be still available (that's why I added
save_bootinfo()).  Perhaps that works because only memory starting
from a rounded-up value of _end will be used, and you're just lucky?
I'm wondering what else it preserves. It sure has to reload the
kernel image, as at least the data section will no longer contain the
initialization values after a reboot...

Laurent?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Laurent Vivier]

Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
> Hi Jason,
> 
> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
>>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
>>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
>>>>>> +               *(u16 *)rng_seed_record->data = len;
>>>
>>> Storing the length should use the proper cpu_to_be16 accessor.
>>
>> Okay, I'll do that for v2.
>>
>> (Simply out of curiosity, why? Isn't m68k always big endian and this
>> is arch/ code?)
> 
> Yes it is.  But virt_parse_bootinfo() below already uses the right
> accessor.
> 
> BTW, I guess people thought the same about PowerPC?
> Although I agree the probability of someone creating a little-endian
> m68k clone in an FPGA or SkyWater project and trying to run Linux on
> it quite low ;-)
> 
>>>> The way I tested this is by having my initramfs just call
>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
>>>> its contents to the console. I checked that it was both present and
>>>> different every time.
>>>
>>> Are you sure the new kernel did receive the same randomness as prepared
>>> by get_random_bytes()? I would expect it to just reboot into qemu,
>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
>>> including generating a new random seed.
>>
>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
> 
> That's interesting.  So QEMU preserves the old bootinfo, which is
> AFAIK not guaranteed to be still available (that's why I added
> save_bootinfo()).  Perhaps that works because only memory starting
> from a rounded-up value of _end will be used, and you're just lucky?
> I'm wondering what else it preserves. It sure has to reload the
> kernel image, as at least the data section will no longer contain the
> initialization values after a reboot...
> 
> Laurent?
>

In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.

I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by 
luck. I will check.

Thanks,
Laurent

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

Hi Laurent,

On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
> > Hi Jason,
> >
> > On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> >>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> >>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> >>>>>> +               *(u16 *)rng_seed_record->data = len;
> >>>
> >>> Storing the length should use the proper cpu_to_be16 accessor.
> >>
> >> Okay, I'll do that for v2.
> >>
> >> (Simply out of curiosity, why? Isn't m68k always big endian and this
> >> is arch/ code?)
> >
> > Yes it is.  But virt_parse_bootinfo() below already uses the right
> > accessor.
> >
> > BTW, I guess people thought the same about PowerPC?
> > Although I agree the probability of someone creating a little-endian
> > m68k clone in an FPGA or SkyWater project and trying to run Linux on
> > it quite low ;-)
> >
> >>>> The way I tested this is by having my initramfs just call
> >>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> >>>> its contents to the console. I checked that it was both present and
> >>>> different every time.
> >>>
> >>> Are you sure the new kernel did receive the same randomness as prepared
> >>> by get_random_bytes()? I would expect it to just reboot into qemu,
> >>> reload the kernel from disk, and recreate a new bootinfo from scratch,
> >>> including generating a new random seed.
> >>
> >> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
> >
> > That's interesting.  So QEMU preserves the old bootinfo, which is
> > AFAIK not guaranteed to be still available (that's why I added
> > save_bootinfo()).  Perhaps that works because only memory starting
> > from a rounded-up value of _end will be used, and you're just lucky?
> > I'm wondering what else it preserves. It sure has to reload the
> > kernel image, as at least the data section will no longer contain the
> > initialization values after a reboot...
> >
> > Laurent?
> >
>
> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
>
> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
> luck. I will check.
>
> Thanks,
> Laurent

Are you sure about that? Or at least, could you point me to where you
think this happens? I'm not as familiar as you with this code base,
but I really am not seeing it. So far as I can tell, on reset, the pc
and stack are reset to their initial places, after TCG resets the cpu
registers to a known state. But the kernel is not reloaded. The same
thing that was in memory before is used again.

Jason

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Laurent Vivier]

Hi Jason,

Le 26/09/2022 à 14:02, Jason A. Donenfeld a écrit :
> Hi Laurent,
> 
> On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
>>> Hi Jason,
>>>
>>> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
>>>>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
>>>>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
>>>>>>>> +               *(u16 *)rng_seed_record->data = len;
>>>>>
>>>>> Storing the length should use the proper cpu_to_be16 accessor.
>>>>
>>>> Okay, I'll do that for v2.
>>>>
>>>> (Simply out of curiosity, why? Isn't m68k always big endian and this
>>>> is arch/ code?)
>>>
>>> Yes it is.  But virt_parse_bootinfo() below already uses the right
>>> accessor.
>>>
>>> BTW, I guess people thought the same about PowerPC?
>>> Although I agree the probability of someone creating a little-endian
>>> m68k clone in an FPGA or SkyWater project and trying to run Linux on
>>> it quite low ;-)
>>>
>>>>>> The way I tested this is by having my initramfs just call
>>>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
>>>>>> its contents to the console. I checked that it was both present and
>>>>>> different every time.
>>>>>
>>>>> Are you sure the new kernel did receive the same randomness as prepared
>>>>> by get_random_bytes()? I would expect it to just reboot into qemu,
>>>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
>>>>> including generating a new random seed.
>>>>
>>>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
>>>
>>> That's interesting.  So QEMU preserves the old bootinfo, which is
>>> AFAIK not guaranteed to be still available (that's why I added
>>> save_bootinfo()).  Perhaps that works because only memory starting
>>> from a rounded-up value of _end will be used, and you're just lucky?
>>> I'm wondering what else it preserves. It sure has to reload the
>>> kernel image, as at least the data section will no longer contain the
>>> initialization values after a reboot...
>>>
>>> Laurent?
>>>
>>
>> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
>>
>> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
>> luck. I will check.
>>
>> Thanks,
>> Laurent
> 
> Are you sure about that? Or at least, could you point me to where you
> think this happens? I'm not as familiar as you with this code base,
> but I really am not seeing it. So far as I can tell, on reset, the pc
> and stack are reset to their initial places, after TCG resets the cpu
> registers to a known state. But the kernel is not reloaded. The same
> thing that was in memory before is used again.

Yes, this is not clear in QEMU but I think this happens in rom_reset():

hw/core/loader.c

1180         if (rom->mr) {
1181             void *host = memory_region_get_ram_ptr(rom->mr);
1182             memcpy(host, rom->data, rom->datasize);
1183             memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
1184         } else {
1185             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
1186                                     rom->data, rom->datasize);
1187             address_space_set(rom->as, rom->addr + rom->datasize, 0,
1188                               rom->romsize - rom->datasize,
1189                               MEMTXATTRS_UNSPECIFIED);
1190         }

kernel and initrd are loaded with load_elf() and load_image_targphys() only once at startup by the 
machine init function (virt_init()).

rom_add_elf_program() adds the kernel to the ROM list
(in include/hw/elf_ops.h, glue(load_elf, SZ) that generates load_elf32() when SZ is 32...)

rom_add_file() adds the initrd to the ROM list too.

And ROMs are restored on reset from these copies by rom_reset().

rom_reset() is registered as a reset handler with qemu_register_reset() by 
rom_check_and_register_reset() at the end of the machine creation by qdev_machine_creation_done().

So I think bootinfo are not restored because there is no such function calls. Perhaps they are saved 
and restaured if they are stored in address space of one of the previous registered ROM.

Thanks,
Laurent

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

Hi Laurent,

On Mon, Sep 26, 2022 at 2:52 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Hi Jason,
>
> Le 26/09/2022 à 14:02, Jason A. Donenfeld a écrit :
> > Hi Laurent,
> >
> > On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
> >>> Hi Jason,
> >>>
> >>> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> >>>>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> >>>>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> >>>>>>>> +               *(u16 *)rng_seed_record->data = len;
> >>>>>
> >>>>> Storing the length should use the proper cpu_to_be16 accessor.
> >>>>
> >>>> Okay, I'll do that for v2.
> >>>>
> >>>> (Simply out of curiosity, why? Isn't m68k always big endian and this
> >>>> is arch/ code?)
> >>>
> >>> Yes it is.  But virt_parse_bootinfo() below already uses the right
> >>> accessor.
> >>>
> >>> BTW, I guess people thought the same about PowerPC?
> >>> Although I agree the probability of someone creating a little-endian
> >>> m68k clone in an FPGA or SkyWater project and trying to run Linux on
> >>> it quite low ;-)
> >>>
> >>>>>> The way I tested this is by having my initramfs just call
> >>>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> >>>>>> its contents to the console. I checked that it was both present and
> >>>>>> different every time.
> >>>>>
> >>>>> Are you sure the new kernel did receive the same randomness as prepared
> >>>>> by get_random_bytes()? I would expect it to just reboot into qemu,
> >>>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
> >>>>> including generating a new random seed.
> >>>>
> >>>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
> >>>
> >>> That's interesting.  So QEMU preserves the old bootinfo, which is
> >>> AFAIK not guaranteed to be still available (that's why I added
> >>> save_bootinfo()).  Perhaps that works because only memory starting
> >>> from a rounded-up value of _end will be used, and you're just lucky?
> >>> I'm wondering what else it preserves. It sure has to reload the
> >>> kernel image, as at least the data section will no longer contain the
> >>> initialization values after a reboot...
> >>>
> >>> Laurent?
> >>>
> >>
> >> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
> >>
> >> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
> >> luck. I will check.
> >>
> >> Thanks,
> >> Laurent
> >
> > Are you sure about that? Or at least, could you point me to where you
> > think this happens? I'm not as familiar as you with this code base,
> > but I really am not seeing it. So far as I can tell, on reset, the pc
> > and stack are reset to their initial places, after TCG resets the cpu
> > registers to a known state. But the kernel is not reloaded. The same
> > thing that was in memory before is used again.
>
> Yes, this is not clear in QEMU but I think this happens in rom_reset():
>
> hw/core/loader.c
>
> 1180         if (rom->mr) {
> 1181             void *host = memory_region_get_ram_ptr(rom->mr);
> 1182             memcpy(host, rom->data, rom->datasize);
> 1183             memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
> 1184         } else {
> 1185             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
> 1186                                     rom->data, rom->datasize);
> 1187             address_space_set(rom->as, rom->addr + rom->datasize, 0,
> 1188                               rom->romsize - rom->datasize,
> 1189                               MEMTXATTRS_UNSPECIFIED);
> 1190         }
>
> kernel and initrd are loaded with load_elf() and load_image_targphys() only once at startup by the
> machine init function (virt_init()).
>
> rom_add_elf_program() adds the kernel to the ROM list
> (in include/hw/elf_ops.h, glue(load_elf, SZ) that generates load_elf32() when SZ is 32...)
>
> rom_add_file() adds the initrd to the ROM list too.
>
> And ROMs are restored on reset from these copies by rom_reset().
>
> rom_reset() is registered as a reset handler with qemu_register_reset() by
> rom_check_and_register_reset() at the end of the machine creation by qdev_machine_creation_done().
>
> So I think bootinfo are not restored because there is no such function calls. Perhaps they are saved
> and restaured if they are stored in address space of one of the previous registered ROM.

Ahh interesting, thanks for the explanation.

So from my debugging, bootinfo is *not* restored, and the previous one
appears to be used. Fortunately it's intact and everything works well
on a reboot.

With that in mind, we now we have to decide whether to:
A) Go with my linux patch to write the rng seed before rebooting (3/3
in v4 of that series).
B) Not go with the linux patch, but instead make sure bootinfo is
restored to its previous value, and then also register a qemu reboot
notifier to refresh the seed in it, like what x86 does.

(A) sounds a lot easier to me. Opinions?

Jason

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Laurent Vivier]

Le 26/09/2022 à 14:56, Jason A. Donenfeld a écrit :
> Hi Laurent,
> 
> On Mon, Sep 26, 2022 at 2:52 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Hi Jason,
>>
>> Le 26/09/2022 à 14:02, Jason A. Donenfeld a écrit :
>>> Hi Laurent,
>>>
>>> On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>>
>>>> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
>>>>> Hi Jason,
>>>>>
>>>>> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
>>>>>>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
>>>>>>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
>>>>>>>>>> +               *(u16 *)rng_seed_record->data = len;
>>>>>>>
>>>>>>> Storing the length should use the proper cpu_to_be16 accessor.
>>>>>>
>>>>>> Okay, I'll do that for v2.
>>>>>>
>>>>>> (Simply out of curiosity, why? Isn't m68k always big endian and this
>>>>>> is arch/ code?)
>>>>>
>>>>> Yes it is.  But virt_parse_bootinfo() below already uses the right
>>>>> accessor.
>>>>>
>>>>> BTW, I guess people thought the same about PowerPC?
>>>>> Although I agree the probability of someone creating a little-endian
>>>>> m68k clone in an FPGA or SkyWater project and trying to run Linux on
>>>>> it quite low ;-)
>>>>>
>>>>>>>> The way I tested this is by having my initramfs just call
>>>>>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
>>>>>>>> its contents to the console. I checked that it was both present and
>>>>>>>> different every time.
>>>>>>>
>>>>>>> Are you sure the new kernel did receive the same randomness as prepared
>>>>>>> by get_random_bytes()? I would expect it to just reboot into qemu,
>>>>>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
>>>>>>> including generating a new random seed.
>>>>>>
>>>>>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
>>>>>
>>>>> That's interesting.  So QEMU preserves the old bootinfo, which is
>>>>> AFAIK not guaranteed to be still available (that's why I added
>>>>> save_bootinfo()).  Perhaps that works because only memory starting
>>>>> from a rounded-up value of _end will be used, and you're just lucky?
>>>>> I'm wondering what else it preserves. It sure has to reload the
>>>>> kernel image, as at least the data section will no longer contain the
>>>>> initialization values after a reboot...
>>>>>
>>>>> Laurent?
>>>>>
>>>>
>>>> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
>>>>
>>>> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
>>>> luck. I will check.
>>>>
>>>> Thanks,
>>>> Laurent
>>>
>>> Are you sure about that? Or at least, could you point me to where you
>>> think this happens? I'm not as familiar as you with this code base,
>>> but I really am not seeing it. So far as I can tell, on reset, the pc
>>> and stack are reset to their initial places, after TCG resets the cpu
>>> registers to a known state. But the kernel is not reloaded. The same
>>> thing that was in memory before is used again.
>>
>> Yes, this is not clear in QEMU but I think this happens in rom_reset():
>>
>> hw/core/loader.c
>>
>> 1180         if (rom->mr) {
>> 1181             void *host = memory_region_get_ram_ptr(rom->mr);
>> 1182             memcpy(host, rom->data, rom->datasize);
>> 1183             memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
>> 1184         } else {
>> 1185             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
>> 1186                                     rom->data, rom->datasize);
>> 1187             address_space_set(rom->as, rom->addr + rom->datasize, 0,
>> 1188                               rom->romsize - rom->datasize,
>> 1189                               MEMTXATTRS_UNSPECIFIED);
>> 1190         }
>>
>> kernel and initrd are loaded with load_elf() and load_image_targphys() only once at startup by the
>> machine init function (virt_init()).
>>
>> rom_add_elf_program() adds the kernel to the ROM list
>> (in include/hw/elf_ops.h, glue(load_elf, SZ) that generates load_elf32() when SZ is 32...)
>>
>> rom_add_file() adds the initrd to the ROM list too.
>>
>> And ROMs are restored on reset from these copies by rom_reset().
>>
>> rom_reset() is registered as a reset handler with qemu_register_reset() by
>> rom_check_and_register_reset() at the end of the machine creation by qdev_machine_creation_done().
>>
>> So I think bootinfo are not restored because there is no such function calls. Perhaps they are saved
>> and restaured if they are stored in address space of one of the previous registered ROM.
> 
> Ahh interesting, thanks for the explanation.
> 
> So from my debugging, bootinfo is *not* restored, and the previous one
> appears to be used. Fortunately it's intact and everything works well
> on a reboot.
> 
> With that in mind, we now we have to decide whether to:
> A) Go with my linux patch to write the rng seed before rebooting (3/3
> in v4 of that series).
> B) Not go with the linux patch, but instead make sure bootinfo is
> restored to its previous value, and then also register a qemu reboot
> notifier to refresh the seed in it, like what x86 does.
> 

I prefer B :)
It's cleaner and under QEMU control.

Thanks,
Laurent
> (A) sounds a lot easier to me. Opinions?
> 
> Jason

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

On Mon, Sep 26, 2022 at 3:02 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 26/09/2022 à 14:56, Jason A. Donenfeld a écrit :
> > Hi Laurent,
> >
> > On Mon, Sep 26, 2022 at 2:52 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> Hi Jason,
> >>
> >> Le 26/09/2022 à 14:02, Jason A. Donenfeld a écrit :
> >>> Hi Laurent,
> >>>
> >>> On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>>>
> >>>> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
> >>>>> Hi Jason,
> >>>>>
> >>>>> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>>>>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> >>>>>>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> >>>>>>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> >>>>>>>>>> +               *(u16 *)rng_seed_record->data = len;
> >>>>>>>
> >>>>>>> Storing the length should use the proper cpu_to_be16 accessor.
> >>>>>>
> >>>>>> Okay, I'll do that for v2.
> >>>>>>
> >>>>>> (Simply out of curiosity, why? Isn't m68k always big endian and this
> >>>>>> is arch/ code?)
> >>>>>
> >>>>> Yes it is.  But virt_parse_bootinfo() below already uses the right
> >>>>> accessor.
> >>>>>
> >>>>> BTW, I guess people thought the same about PowerPC?
> >>>>> Although I agree the probability of someone creating a little-endian
> >>>>> m68k clone in an FPGA or SkyWater project and trying to run Linux on
> >>>>> it quite low ;-)
> >>>>>
> >>>>>>>> The way I tested this is by having my initramfs just call
> >>>>>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> >>>>>>>> its contents to the console. I checked that it was both present and
> >>>>>>>> different every time.
> >>>>>>>
> >>>>>>> Are you sure the new kernel did receive the same randomness as prepared
> >>>>>>> by get_random_bytes()? I would expect it to just reboot into qemu,
> >>>>>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
> >>>>>>> including generating a new random seed.
> >>>>>>
> >>>>>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
> >>>>>
> >>>>> That's interesting.  So QEMU preserves the old bootinfo, which is
> >>>>> AFAIK not guaranteed to be still available (that's why I added
> >>>>> save_bootinfo()).  Perhaps that works because only memory starting
> >>>>> from a rounded-up value of _end will be used, and you're just lucky?
> >>>>> I'm wondering what else it preserves. It sure has to reload the
> >>>>> kernel image, as at least the data section will no longer contain the
> >>>>> initialization values after a reboot...
> >>>>>
> >>>>> Laurent?
> >>>>>
> >>>>
> >>>> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
> >>>>
> >>>> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
> >>>> luck. I will check.
> >>>>
> >>>> Thanks,
> >>>> Laurent
> >>>
> >>> Are you sure about that? Or at least, could you point me to where you
> >>> think this happens? I'm not as familiar as you with this code base,
> >>> but I really am not seeing it. So far as I can tell, on reset, the pc
> >>> and stack are reset to their initial places, after TCG resets the cpu
> >>> registers to a known state. But the kernel is not reloaded. The same
> >>> thing that was in memory before is used again.
> >>
> >> Yes, this is not clear in QEMU but I think this happens in rom_reset():
> >>
> >> hw/core/loader.c
> >>
> >> 1180         if (rom->mr) {
> >> 1181             void *host = memory_region_get_ram_ptr(rom->mr);
> >> 1182             memcpy(host, rom->data, rom->datasize);
> >> 1183             memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
> >> 1184         } else {
> >> 1185             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
> >> 1186                                     rom->data, rom->datasize);
> >> 1187             address_space_set(rom->as, rom->addr + rom->datasize, 0,
> >> 1188                               rom->romsize - rom->datasize,
> >> 1189                               MEMTXATTRS_UNSPECIFIED);
> >> 1190         }
> >>
> >> kernel and initrd are loaded with load_elf() and load_image_targphys() only once at startup by the
> >> machine init function (virt_init()).
> >>
> >> rom_add_elf_program() adds the kernel to the ROM list
> >> (in include/hw/elf_ops.h, glue(load_elf, SZ) that generates load_elf32() when SZ is 32...)
> >>
> >> rom_add_file() adds the initrd to the ROM list too.
> >>
> >> And ROMs are restored on reset from these copies by rom_reset().
> >>
> >> rom_reset() is registered as a reset handler with qemu_register_reset() by
> >> rom_check_and_register_reset() at the end of the machine creation by qdev_machine_creation_done().
> >>
> >> So I think bootinfo are not restored because there is no such function calls. Perhaps they are saved
> >> and restaured if they are stored in address space of one of the previous registered ROM.
> >
> > Ahh interesting, thanks for the explanation.
> >
> > So from my debugging, bootinfo is *not* restored, and the previous one
> > appears to be used. Fortunately it's intact and everything works well
> > on a reboot.
> >
> > With that in mind, we now we have to decide whether to:
> > A) Go with my linux patch to write the rng seed before rebooting (3/3
> > in v4 of that series).
> > B) Not go with the linux patch, but instead make sure bootinfo is
> > restored to its previous value, and then also register a qemu reboot
> > notifier to refresh the seed in it, like what x86 does.
> >
>
> I prefer B :)
> It's cleaner and under QEMU control.

Okay. I'm happy to follow your preference. Just one last question,
though: is this what happens on baremetal bootloaders too? Or does no
such thing really exist so it doesn't matter?

Jason

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Laurent Vivier]

Le 26/09/2022 à 15:04, Jason A. Donenfeld a écrit :
> On Mon, Sep 26, 2022 at 3:02 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 26/09/2022 à 14:56, Jason A. Donenfeld a écrit :
>>> Hi Laurent,
>>>
>>> On Mon, Sep 26, 2022 at 2:52 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>>
>>>> Hi Jason,
>>>>
>>>> Le 26/09/2022 à 14:02, Jason A. Donenfeld a écrit :
>>>>> Hi Laurent,
>>>>>
>>>>> On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
>>>>>>
>>>>>> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
>>>>>>> Hi Jason,
>>>>>>>
>>>>>>> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>>>>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>>>>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
>>>>>>>>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
>>>>>>>>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
>>>>>>>>>>>> +               *(u16 *)rng_seed_record->data = len;
>>>>>>>>>
>>>>>>>>> Storing the length should use the proper cpu_to_be16 accessor.
>>>>>>>>
>>>>>>>> Okay, I'll do that for v2.
>>>>>>>>
>>>>>>>> (Simply out of curiosity, why? Isn't m68k always big endian and this
>>>>>>>> is arch/ code?)
>>>>>>>
>>>>>>> Yes it is.  But virt_parse_bootinfo() below already uses the right
>>>>>>> accessor.
>>>>>>>
>>>>>>> BTW, I guess people thought the same about PowerPC?
>>>>>>> Although I agree the probability of someone creating a little-endian
>>>>>>> m68k clone in an FPGA or SkyWater project and trying to run Linux on
>>>>>>> it quite low ;-)
>>>>>>>
>>>>>>>>>> The way I tested this is by having my initramfs just call
>>>>>>>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
>>>>>>>>>> its contents to the console. I checked that it was both present and
>>>>>>>>>> different every time.
>>>>>>>>>
>>>>>>>>> Are you sure the new kernel did receive the same randomness as prepared
>>>>>>>>> by get_random_bytes()? I would expect it to just reboot into qemu,
>>>>>>>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
>>>>>>>>> including generating a new random seed.
>>>>>>>>
>>>>>>>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
>>>>>>>
>>>>>>> That's interesting.  So QEMU preserves the old bootinfo, which is
>>>>>>> AFAIK not guaranteed to be still available (that's why I added
>>>>>>> save_bootinfo()).  Perhaps that works because only memory starting
>>>>>>> from a rounded-up value of _end will be used, and you're just lucky?
>>>>>>> I'm wondering what else it preserves. It sure has to reload the
>>>>>>> kernel image, as at least the data section will no longer contain the
>>>>>>> initialization values after a reboot...
>>>>>>>
>>>>>>> Laurent?
>>>>>>>
>>>>>>
>>>>>> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
>>>>>>
>>>>>> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
>>>>>> luck. I will check.
>>>>>>
>>>>>> Thanks,
>>>>>> Laurent
>>>>>
>>>>> Are you sure about that? Or at least, could you point me to where you
>>>>> think this happens? I'm not as familiar as you with this code base,
>>>>> but I really am not seeing it. So far as I can tell, on reset, the pc
>>>>> and stack are reset to their initial places, after TCG resets the cpu
>>>>> registers to a known state. But the kernel is not reloaded. The same
>>>>> thing that was in memory before is used again.
>>>>
>>>> Yes, this is not clear in QEMU but I think this happens in rom_reset():
>>>>
>>>> hw/core/loader.c
>>>>
>>>> 1180         if (rom->mr) {
>>>> 1181             void *host = memory_region_get_ram_ptr(rom->mr);
>>>> 1182             memcpy(host, rom->data, rom->datasize);
>>>> 1183             memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
>>>> 1184         } else {
>>>> 1185             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
>>>> 1186                                     rom->data, rom->datasize);
>>>> 1187             address_space_set(rom->as, rom->addr + rom->datasize, 0,
>>>> 1188                               rom->romsize - rom->datasize,
>>>> 1189                               MEMTXATTRS_UNSPECIFIED);
>>>> 1190         }
>>>>
>>>> kernel and initrd are loaded with load_elf() and load_image_targphys() only once at startup by the
>>>> machine init function (virt_init()).
>>>>
>>>> rom_add_elf_program() adds the kernel to the ROM list
>>>> (in include/hw/elf_ops.h, glue(load_elf, SZ) that generates load_elf32() when SZ is 32...)
>>>>
>>>> rom_add_file() adds the initrd to the ROM list too.
>>>>
>>>> And ROMs are restored on reset from these copies by rom_reset().
>>>>
>>>> rom_reset() is registered as a reset handler with qemu_register_reset() by
>>>> rom_check_and_register_reset() at the end of the machine creation by qdev_machine_creation_done().
>>>>
>>>> So I think bootinfo are not restored because there is no such function calls. Perhaps they are saved
>>>> and restaured if they are stored in address space of one of the previous registered ROM.
>>>
>>> Ahh interesting, thanks for the explanation.
>>>
>>> So from my debugging, bootinfo is *not* restored, and the previous one
>>> appears to be used. Fortunately it's intact and everything works well
>>> on a reboot.
>>>
>>> With that in mind, we now we have to decide whether to:
>>> A) Go with my linux patch to write the rng seed before rebooting (3/3
>>> in v4 of that series).
>>> B) Not go with the linux patch, but instead make sure bootinfo is
>>> restored to its previous value, and then also register a qemu reboot
>>> notifier to refresh the seed in it, like what x86 does.
>>>
>>
>> I prefer B :)
>> It's cleaner and under QEMU control.
> 
> Okay. I'm happy to follow your preference. Just one last question,
> though: is this what happens on baremetal bootloaders too? Or does no
> such thing really exist so it doesn't matter?

With a baremetal bootloader information are provided by the firmware.

In our case, we don't have bootloader nor firmware as the kernel is loaded and started by QEMU.
So QEMU must do the bootloader and the firmware actions. It's why I prefer B.

Thanks,
LAurent

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

Hi Laurent,

On Mon, Sep 26, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 26/09/2022 à 15:04, Jason A. Donenfeld a écrit :
> > On Mon, Sep 26, 2022 at 3:02 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> Le 26/09/2022 à 14:56, Jason A. Donenfeld a écrit :
> >>> Hi Laurent,
> >>>
> >>> On Mon, Sep 26, 2022 at 2:52 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>>>
> >>>> Hi Jason,
> >>>>
> >>>> Le 26/09/2022 à 14:02, Jason A. Donenfeld a écrit :
> >>>>> Hi Laurent,
> >>>>>
> >>>>> On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >>>>>>
> >>>>>> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
> >>>>>>> Hi Jason,
> >>>>>>>
> >>>>>>> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>>>>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>>>>>>>>>> +       if (rng_seed_record && rng_seed_record->size > sizeof(*rng_seed_record) + 2) {
> >>>>>>>>>>>> +               u16 len = rng_seed_record->size - sizeof(*rng_seed_record) - 2;
> >>>>>>>>>>>> +               get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> >>>>>>>>>>>> +               *(u16 *)rng_seed_record->data = len;
> >>>>>>>>>
> >>>>>>>>> Storing the length should use the proper cpu_to_be16 accessor.
> >>>>>>>>
> >>>>>>>> Okay, I'll do that for v2.
> >>>>>>>>
> >>>>>>>> (Simply out of curiosity, why? Isn't m68k always big endian and this
> >>>>>>>> is arch/ code?)
> >>>>>>>
> >>>>>>> Yes it is.  But virt_parse_bootinfo() below already uses the right
> >>>>>>> accessor.
> >>>>>>>
> >>>>>>> BTW, I guess people thought the same about PowerPC?
> >>>>>>> Although I agree the probability of someone creating a little-endian
> >>>>>>> m68k clone in an FPGA or SkyWater project and trying to run Linux on
> >>>>>>> it quite low ;-)
> >>>>>>>
> >>>>>>>>>> The way I tested this is by having my initramfs just call
> >>>>>>>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> >>>>>>>>>> its contents to the console. I checked that it was both present and
> >>>>>>>>>> different every time.
> >>>>>>>>>
> >>>>>>>>> Are you sure the new kernel did receive the same randomness as prepared
> >>>>>>>>> by get_random_bytes()? I would expect it to just reboot into qemu,
> >>>>>>>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
> >>>>>>>>> including generating a new random seed.
> >>>>>>>>
> >>>>>>>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
> >>>>>>>
> >>>>>>> That's interesting.  So QEMU preserves the old bootinfo, which is
> >>>>>>> AFAIK not guaranteed to be still available (that's why I added
> >>>>>>> save_bootinfo()).  Perhaps that works because only memory starting
> >>>>>>> from a rounded-up value of _end will be used, and you're just lucky?
> >>>>>>> I'm wondering what else it preserves. It sure has to reload the
> >>>>>>> kernel image, as at least the data section will no longer contain the
> >>>>>>> initialization values after a reboot...
> >>>>>>>
> >>>>>>> Laurent?
> >>>>>>>
> >>>>>>
> >>>>>> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
> >>>>>>
> >>>>>> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
> >>>>>> luck. I will check.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Laurent
> >>>>>
> >>>>> Are you sure about that? Or at least, could you point me to where you
> >>>>> think this happens? I'm not as familiar as you with this code base,
> >>>>> but I really am not seeing it. So far as I can tell, on reset, the pc
> >>>>> and stack are reset to their initial places, after TCG resets the cpu
> >>>>> registers to a known state. But the kernel is not reloaded. The same
> >>>>> thing that was in memory before is used again.
> >>>>
> >>>> Yes, this is not clear in QEMU but I think this happens in rom_reset():
> >>>>
> >>>> hw/core/loader.c
> >>>>
> >>>> 1180         if (rom->mr) {
> >>>> 1181             void *host = memory_region_get_ram_ptr(rom->mr);
> >>>> 1182             memcpy(host, rom->data, rom->datasize);
> >>>> 1183             memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
> >>>> 1184         } else {
> >>>> 1185             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
> >>>> 1186                                     rom->data, rom->datasize);
> >>>> 1187             address_space_set(rom->as, rom->addr + rom->datasize, 0,
> >>>> 1188                               rom->romsize - rom->datasize,
> >>>> 1189                               MEMTXATTRS_UNSPECIFIED);
> >>>> 1190         }
> >>>>
> >>>> kernel and initrd are loaded with load_elf() and load_image_targphys() only once at startup by the
> >>>> machine init function (virt_init()).
> >>>>
> >>>> rom_add_elf_program() adds the kernel to the ROM list
> >>>> (in include/hw/elf_ops.h, glue(load_elf, SZ) that generates load_elf32() when SZ is 32...)
> >>>>
> >>>> rom_add_file() adds the initrd to the ROM list too.
> >>>>
> >>>> And ROMs are restored on reset from these copies by rom_reset().
> >>>>
> >>>> rom_reset() is registered as a reset handler with qemu_register_reset() by
> >>>> rom_check_and_register_reset() at the end of the machine creation by qdev_machine_creation_done().
> >>>>
> >>>> So I think bootinfo are not restored because there is no such function calls. Perhaps they are saved
> >>>> and restaured if they are stored in address space of one of the previous registered ROM.
> >>>
> >>> Ahh interesting, thanks for the explanation.
> >>>
> >>> So from my debugging, bootinfo is *not* restored, and the previous one
> >>> appears to be used. Fortunately it's intact and everything works well
> >>> on a reboot.
> >>>
> >>> With that in mind, we now we have to decide whether to:
> >>> A) Go with my linux patch to write the rng seed before rebooting (3/3
> >>> in v4 of that series).
> >>> B) Not go with the linux patch, but instead make sure bootinfo is
> >>> restored to its previous value, and then also register a qemu reboot
> >>> notifier to refresh the seed in it, like what x86 does.
> >>>
> >>
> >> I prefer B :)
> >> It's cleaner and under QEMU control.
> >
> > Okay. I'm happy to follow your preference. Just one last question,
> > though: is this what happens on baremetal bootloaders too? Or does no
> > such thing really exist so it doesn't matter?
>
> With a baremetal bootloader information are provided by the firmware.
>
> In our case, we don't have bootloader nor firmware as the kernel is loaded and started by QEMU.
> So QEMU must do the bootloader and the firmware actions. It's why I prefer B.

Okay, so on reboot, control goes back to firmware, which then supplies
fresh arguments and such. So QEMU should do the same. Makes sense.

Jason

Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

[Geert Uytterhoeven]

Hi Jason,

On Mon, Sep 26, 2022 at 3:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Mon, Sep 26, 2022 at 3:02 PM Laurent Vivier <laurent@vivier.eu> wrote:
> > Le 26/09/2022 à 14:56, Jason A. Donenfeld a écrit :
> > > On Mon, Sep 26, 2022 at 2:52 PM Laurent Vivier <laurent@vivier.eu> wrote:
> > >> Le 26/09/2022 à 14:02, Jason A. Donenfeld a écrit :
> > >>> On Fri, Sep 23, 2022 at 3:10 PM Laurent Vivier <laurent@vivier.eu> wrote:
> > >>>> Le 23/09/2022 à 14:50, Geert Uytterhoeven a écrit :
> > >>>>> On Fri, Sep 23, 2022 at 2:26 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >>>>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >>>>>>>> The way I tested this is by having my initramfs just call
> > >>>>>>>> `reboot(RB_AUTOBOOT);`, and having add_bootloader_randomness() print
> > >>>>>>>> its contents to the console. I checked that it was both present and
> > >>>>>>>> different every time.
> > >>>>>>>
> > >>>>>>> Are you sure the new kernel did receive the same randomness as prepared
> > >>>>>>> by get_random_bytes()? I would expect it to just reboot into qemu,
> > >>>>>>> reload the kernel from disk, and recreate a new bootinfo from scratch,
> > >>>>>>> including generating a new random seed.
> > >>>>>>
> > >>>>>> Yes I'm sure. Without this patch, the new kernel sees the zeroed state.
> > >>>>>
> > >>>>> That's interesting.  So QEMU preserves the old bootinfo, which is
> > >>>>> AFAIK not guaranteed to be still available (that's why I added
> > >>>>> save_bootinfo()).  Perhaps that works because only memory starting
> > >>>>> from a rounded-up value of _end will be used, and you're just lucky?
> > >>>>> I'm wondering what else it preserves. It sure has to reload the
> > >>>>> kernel image, as at least the data section will no longer contain the
> > >>>>> initialization values after a reboot...
> > >>>>>
> > >>>>> Laurent?
> > >>>>>
> > >>>>
> > >>>> In QEMU the loader makes a copy of the kernel and the initrd and this copy is restored on a reset.
> > >>>>
> > >>>> I don't think there is a mechanism in QEMU to save the BOOTINFO section, so I think it works by
> > >>>> luck. I will check.
> > >>>>
> > >>>> Thanks,
> > >>>> Laurent
> > >>>
> > >>> Are you sure about that? Or at least, could you point me to where you
> > >>> think this happens? I'm not as familiar as you with this code base,
> > >>> but I really am not seeing it. So far as I can tell, on reset, the pc
> > >>> and stack are reset to their initial places, after TCG resets the cpu
> > >>> registers to a known state. But the kernel is not reloaded. The same
> > >>> thing that was in memory before is used again.
> > >>
> > >> Yes, this is not clear in QEMU but I think this happens in rom_reset():
> > >>
> > >> hw/core/loader.c
> > >>
> > >> 1180         if (rom->mr) {
> > >> 1181             void *host = memory_region_get_ram_ptr(rom->mr);
> > >> 1182             memcpy(host, rom->data, rom->datasize);
> > >> 1183             memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
> > >> 1184         } else {
> > >> 1185             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
> > >> 1186                                     rom->data, rom->datasize);
> > >> 1187             address_space_set(rom->as, rom->addr + rom->datasize, 0,
> > >> 1188                               rom->romsize - rom->datasize,
> > >> 1189                               MEMTXATTRS_UNSPECIFIED);
> > >> 1190         }
> > >>
> > >> kernel and initrd are loaded with load_elf() and load_image_targphys() only once at startup by the
> > >> machine init function (virt_init()).
> > >>
> > >> rom_add_elf_program() adds the kernel to the ROM list
> > >> (in include/hw/elf_ops.h, glue(load_elf, SZ) that generates load_elf32() when SZ is 32...)
> > >>
> > >> rom_add_file() adds the initrd to the ROM list too.
> > >>
> > >> And ROMs are restored on reset from these copies by rom_reset().
> > >>
> > >> rom_reset() is registered as a reset handler with qemu_register_reset() by
> > >> rom_check_and_register_reset() at the end of the machine creation by qdev_machine_creation_done().
> > >>
> > >> So I think bootinfo are not restored because there is no such function calls. Perhaps they are saved
> > >> and restaured if they are stored in address space of one of the previous registered ROM.
> > >
> > > Ahh interesting, thanks for the explanation.
> > >
> > > So from my debugging, bootinfo is *not* restored, and the previous one
> > > appears to be used. Fortunately it's intact and everything works well
> > > on a reboot.
> > >
> > > With that in mind, we now we have to decide whether to:
> > > A) Go with my linux patch to write the rng seed before rebooting (3/3
> > > in v4 of that series).
> > > B) Not go with the linux patch, but instead make sure bootinfo is
> > > restored to its previous value, and then also register a qemu reboot
> > > notifier to refresh the seed in it, like what x86 does.
> >
> > I prefer B :)
> > It's cleaner and under QEMU control.

Me too.

> Okay. I'm happy to follow your preference. Just one last question,
> though: is this what happens on baremetal bootloaders too? Or does no
> such thing really exist so it doesn't matter?

On e.g. Amiga, the whole system is reset, and starts AmigaOS from ROM.
So for booting Linux, you go through the whole Linux bootstrap chain,
loading kernel and optional ramdisk, preparing bootinfo, and passing
all of that to Linux. It should be the same on other real platforms.

ARAnyM has its own bootstrap, and handles reboot itself, but it seems
to behave the same, as it reloads a new kernel from disk on reboot.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds