Message-ID: <e0ee39d5-33e9-c7ae-a1e9-3b21e83c70fd@vivier.eu>
Date:   Mon, 26 Sep 2022 15:02:42 +0200
From:   Laurent Vivier <laurent@...ier.eu>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        linux-m68k@...ts.linux-m68k.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot

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@...ier.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@...ier.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@...c4.com> wrote:
>>>>>> On Fri, Sep 23, 2022 at 2:23 PM Geert Uytterhoeven <geert@...ux-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

Powered by blists - more mailing lists