Messages in this thread | | | From | "Jason A. Donenfeld" <> | Date | Mon, 26 Sep 2022 15:12:01 +0200 | Subject | Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot |
| |
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
| |