Re: [PATCH] m68k: virt: pass RNG seed via bootinfo block

[Jason A. Donenfeld]

Hi Geert,

On Sun, Jun 26, 2022 at 11:39:46AM +0200, Geert Uytterhoeven wrote:
> Hi Jason,
> 
> Thanks for your patch!
> 
> On Sat, Jun 25, 2022 at 6:26 PM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
> > On Sat, Jun 25, 2022 at 6:24 PM Laurent Vivier <laurent@xxxxxxxxx> wrote:
> > > Le 25/06/2022 à 18:19, Jason A. Donenfeld a écrit :
> > > > On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <laurent@xxxxxxxxx> wrote:
> > > >> Le 25/06/2022 à 17:38, Jason A. Donenfeld a écrit :
> > > >>> Other virt VMs can pass RNG seeds via the "rng-seed" device tree
> 
> FTR, "rng-seed" does not seem to be documented anywhere, not under
> Documentation/devicetree/bindings/, and not in the Devicetree
> Specification?

Good point. It's quite old, this field, so odd it was missed. I'll send
in a separate patch for that.

> 
> > > >>> property or via UEFI, but m68k doesn't have either. Instead it has its
> > > >>> own bootinfo protocol. So this commit adds support for receiving a RNG
> > > >>> seed from it, which will be used at the earliest possible time in boot,
> > > >>> just like device tree.
> > > >>>
> > > >>> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> > > >>> --- a/arch/m68k/include/uapi/asm/bootinfo-virt.h
> > > >>> +++ b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> > > >>> @@ -12,6 +12,7 @@
> > > >>>    #define BI_VIRT_GF_TTY_BASE 0x8003
> > > >>>    #define BI_VIRT_VIRTIO_BASE 0x8004
> > > >>>    #define BI_VIRT_CTRL_BASE   0x8005
> > > >>> +#define BI_VIRT_RNG_SEED     0x8006
> 
> Please add a comment documenting the record format.

Ack.

> 
> Laurent: Would be nice if you could add this for the other BI_*, too.
> 
> > > >>>
> > > >>>    #define VIRT_BOOTI_VERSION  MK_BI_VERSION(2, 0)
> > > >>>
> > > >>> diff --git a/arch/m68k/virt/config.c b/arch/m68k/virt/config.c
> > > >>> index 632ba200ad42..ad71af8273ec 100644
> > > >>> --- a/arch/m68k/virt/config.c
> > > >>> +++ b/arch/m68k/virt/config.c
> > > >>> @@ -2,6 +2,7 @@
> > > >>>
> > > >>>    #include <linux/reboot.h>
> > > >>>    #include <linux/serial_core.h>
> > > >>> +#include <linux/random.h>
> > > >>>    #include <clocksource/timer-goldfish.h>
> > > >>>
> > > >>>    #include <asm/bootinfo.h>
> > > >>> @@ -92,6 +93,9 @@ int __init virt_parse_bootinfo(const struct bi_record *record)
> > > >>>                data += 4;
> > > >>>                virt_bi_data.virtio.irq = be32_to_cpup(data);
> > > >>>                break;
> > > >>> +     case BI_VIRT_RNG_SEED:
> > > >>> +             add_bootloader_randomness(data + 4, be32_to_cpup(data));
> > > >>
> > > >> In fact, why don't you use the record->size to get the size of the buffer?
> > > >>
> > > >> It seems useless to encode twice the length of the buffer, the second time on a 32bit while the
> > > >> length cannot exceed a 16bit value.
> > > >
> > > > Doesn't that make the length ambiguous because of required alignment?
> > >
> > > I agree, it's why I understand reviewing the QEMU part of your patch.
> > >
> > > > Would rather keep this general. As is, it's also much more like the
> > > > others and more uniform to keep it that way. You were able to review
> > > > it and see that it was right after glancing for a second. That seems
> > > > superior to any imaginary gains we'd get by overloading the record
> > > > size.
> > >
> > > And what about using a 16bit field rather than a 32bit field as the encoded length cannot be greater
> > > than the record length?
> >
> > I guess but that's different from all other length fields, and means
> > we can't expand past 65k if somebody wants to use this for something
> > more interesting later. Again I wonder what stinginess here gets us.
> > This is just a boot parameter... No need to go crazy optimizing it.
> 
> You cannot extend this past (64 KiB - sizeof(struct bi_record))
> anyway, as the total record size is limited to 64 KiB, regardless of
> the additional buffer size you try to encode inside your own 32-bit
> size field.
> 
> So either just store the data inside the record, rely on bi_record.size,
> and live with random data that must be a number of even bytes (does
> it really hurt to drop the last byte, or add a dummy byte?), or store
> a pointer/size, like is done for e.g. BI_RAMDISK.

I modeled this on BOOTINFOSTR, which benefits from null termination.
I'll just reduce the length field to 2 bytes. I really don't want to
play padding games here, and anyway the length field needs to be
separate for reasons that will become apparent in v2 (zeroing for
kexec).

Jason