lkml.org 
[lkml]   [2022]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot
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

\
 
 \ /
  Last update: 2022-09-26 16:39    [W:7.372 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site