Other virt VMs can pass RNG seeds via the "rng-seed" device tree
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 <[email protected]>
---
arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
arch/m68k/virt/config.c | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
index e4db7e2213ab..7c3044acdf4a 100644
--- 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
#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));
+ break;
default:
unknown = 1;
break;
--
2.35.1
Le 25/06/2022 à 17:38, Jason A. Donenfeld a écrit :
> Other virt VMs can pass RNG seeds via the "rng-seed" device tree
> 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 <[email protected]>
> ---
> arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
> arch/m68k/virt/config.c | 4 ++++
> 2 files changed, 5 insertions(+)
>
> diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> index e4db7e2213ab..7c3044acdf4a 100644
> --- 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
>
> #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));
> + break;
> default:
> unknown = 1;
> break;
Reviewed-by: Laurent Vivier <[email protected]>
Le 25/06/2022 à 17:38, Jason A. Donenfeld a écrit :
> Other virt VMs can pass RNG seeds via the "rng-seed" device tree
> 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 <[email protected]>
> ---
> arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
> arch/m68k/virt/config.c | 4 ++++
> 2 files changed, 5 insertions(+)
>
> diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> index e4db7e2213ab..7c3044acdf4a 100644
> --- 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
>
> #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.
Thanks,
Laurent
On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <[email protected]> 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
> > 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 <[email protected]>
> > ---
> > arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
> > arch/m68k/virt/config.c | 4 ++++
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> > index e4db7e2213ab..7c3044acdf4a 100644
> > --- 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
> >
> > #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?
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.
Jason
Le 25/06/2022 à 18:19, Jason A. Donenfeld a écrit :
> On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <[email protected]> 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
>>> 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 <[email protected]>
>>> ---
>>> arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
>>> arch/m68k/virt/config.c | 4 ++++
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
>>> index e4db7e2213ab..7c3044acdf4a 100644
>>> --- 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
>>>
>>> #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?
Thanks,
Laurent
Le 25/06/2022 à 18:26, Jason A. Donenfeld a écrit :
> On Sat, Jun 25, 2022 at 6:24 PM Laurent Vivier <[email protected]> wrote:
>>
>> Le 25/06/2022 à 18:19, Jason A. Donenfeld a écrit :
>>> On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <[email protected]> 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
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
>>>>> arch/m68k/virt/config.c | 4 ++++
>>>>> 2 files changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
>>>>> index e4db7e2213ab..7c3044acdf4a 100644
>>>>> --- 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
>>>>>
>>>>> #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.
I agree too.
Thanks,
Laurent
On Sat, Jun 25, 2022 at 6:24 PM Laurent Vivier <[email protected]> wrote:
>
> Le 25/06/2022 à 18:19, Jason A. Donenfeld a écrit :
> > On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <[email protected]> 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
> >>> 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 <[email protected]>
> >>> ---
> >>> arch/m68k/include/uapi/asm/bootinfo-virt.h | 1 +
> >>> arch/m68k/virt/config.c | 4 ++++
> >>> 2 files changed, 5 insertions(+)
> >>>
> >>> diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> >>> index e4db7e2213ab..7c3044acdf4a 100644
> >>> --- 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
> >>>
> >>> #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.
Jason
Hi Jason,
Thanks for your patch!
On Sat, Jun 25, 2022 at 6:26 PM Jason A. Donenfeld <[email protected]> wrote:
> On Sat, Jun 25, 2022 at 6:24 PM Laurent Vivier <[email protected]> wrote:
> > Le 25/06/2022 à 18:19, Jason A. Donenfeld a écrit :
> > > On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <[email protected]> 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?
> > >>> 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 <[email protected]>
> > >>> --- 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.
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.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
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 <[email protected]> wrote:
> > On Sat, Jun 25, 2022 at 6:24 PM Laurent Vivier <[email protected]> wrote:
> > > Le 25/06/2022 à 18:19, Jason A. Donenfeld a écrit :
> > > > On Sat, Jun 25, 2022 at 6:08 PM Laurent Vivier <[email protected]> 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 <[email protected]>
> > > >>> --- 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
Other virt VMs can pass RNG seeds via the "rng-seed" device tree
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.
Reviewed-by: Laurent Vivier <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
arch/m68k/include/uapi/asm/bootinfo-virt.h | 7 +++++++
arch/m68k/virt/config.c | 9 +++++++++
2 files changed, 16 insertions(+)
diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
index e4db7e2213ab..0cb2c2a41610 100644
--- a/arch/m68k/include/uapi/asm/bootinfo-virt.h
+++ b/arch/m68k/include/uapi/asm/bootinfo-virt.h
@@ -13,6 +13,13 @@
#define BI_VIRT_VIRTIO_BASE 0x8004
#define BI_VIRT_CTRL_BASE 0x8005
+/* A random seed used to initialize the RNG. Record format:
+ *
+ * - length [ 2 bytes, 16-bit big endian ]
+ * - seed data [ `length` bytes ]
+ */
+#define BI_VIRT_RNG_SEED 0x8006
+
#define VIRT_BOOTI_VERSION MK_BI_VERSION(2, 0)
#endif /* _UAPI_ASM_M68K_BOOTINFO_MAC_H */
diff --git a/arch/m68k/virt/config.c b/arch/m68k/virt/config.c
index 632ba200ad42..645acc6918b2 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,14 @@ 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: {
+ u16 len = be16_to_cpup(data);
+ add_bootloader_randomness(data + 2, len);
+ /* Zero the data to preserve forward secrecy, and zero the
+ * length to prevent kexec from using it. */
+ memzero_explicit((void *)data, len + 2);
+ break;
+ }
default:
unknown = 1;
break;
--
2.35.1
On Sun, Jun 26, 2022 at 1:15 PM Jason A. Donenfeld <[email protected]> wrote:
> Other virt VMs can pass RNG seeds via the "rng-seed" device tree
> 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.
>
> Reviewed-by: Laurent Vivier <[email protected]>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> --- a/arch/m68k/include/uapi/asm/bootinfo-virt.h
> +++ b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> @@ -13,6 +13,13 @@
> #define BI_VIRT_VIRTIO_BASE 0x8004
> #define BI_VIRT_CTRL_BASE 0x8005
>
> +/* A random seed used to initialize the RNG. Record format:
> + *
> + * - length [ 2 bytes, 16-bit big endian ]
> + * - seed data [ `length` bytes ]
", padded to preserve 2-byte alignment"
Reviewed-by: Geert Uytterhoeven <[email protected]>
i.e. will queue in the m68k for-v5.20 branch with the above fixed.
No need to resend.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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