[PATCH v3 1/3] m68k: process bootinfo records before saving them

[PATCH v3 1/3] m68k: process bootinfo records before saving them

[Jason A. Donenfeld]

The RNG seed boot record is memzeroed after processing, in order to
preserve forward secrecy. By saving the bootinfo for procfs prior to
that, forward secrecy is violated, since it becomes possible to recover
past states. So, save the bootinfo block only after first processing
them.

Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/m68k/kernel/setup_mm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
index e62fa8f2149b..7e7ef67cff8b 100644
--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -109,10 +109,9 @@ extern void paging_init(void);
 
 static void __init m68k_parse_bootinfo(const struct bi_record *record)
 {
+	const struct bi_record *first_record = record;
 	uint16_t tag;
 
-	save_bootinfo(record);
-
 	while ((tag = be16_to_cpu(record->tag)) != BI_LAST) {
 		int unknown = 0;
 		const void *data = record->data;
@@ -182,6 +181,8 @@ static void __init m68k_parse_bootinfo(const struct bi_record *record)
 		record = (struct bi_record *)((unsigned long)record + size);
 	}
 
+	save_bootinfo(first_record);
+
 	m68k_realnum_memory = m68k_num_memory;
 #ifdef CONFIG_SINGLE_MEMORY_CHUNK
 	if (m68k_num_memory > 1) {
-- 
2.37.3

[PATCH v3 2/3] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

Rather than rebooting into a system with no entropy, regenerate the RNG
seed before rebooting, so that the new system has a fresh seed.

Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/m68k/virt/config.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/m68k/virt/config.c b/arch/m68k/virt/config.c
index 4ab22946ff68..d4627840e35b 100644
--- a/arch/m68k/virt/config.c
+++ b/arch/m68k/virt/config.c
@@ -45,10 +45,18 @@ static void virt_halt(void)
 		;
 }
 
+static const struct bi_record *rng_seed_record;
+
 static void virt_reset(void)
 {
 	void __iomem *base = (void __iomem *)virt_bi_data.ctrl.mmio;
 
+	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 = cpu_to_be16(len);
+	}
+
 	iowrite32be(CMD_RESET, base + VIRT_CTRL_REG_CMD);
 	local_irq_disable();
 	while (1)
@@ -101,6 +109,8 @@ int __init virt_parse_bootinfo(const struct bi_record *record)
 		 * length to prevent kexec from using it.
 		 */
 		memzero_explicit((void *)data, len + 2);
+		/* Store a reference to be filled in on reboot. */
+		rng_seed_record = record;
 		break;
 	}
 	default:
-- 
2.37.3

Re: [PATCH v3 2/3] m68k: virt: generate new RNG seed on reboot

[Geert Uytterhoeven]

Hi Jason,

On Fri, Sep 23, 2022 at 7:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Rather than rebooting into a system with no entropy, regenerate the RNG
> seed before rebooting, so that the new system has a fresh seed.
>
> Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Thanks for your patch!

I still doubt this is actually guaranteed to work, as the memory containing
the bootinfo might be overwritten during normal operation.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v3 2/3] m68k: virt: generate new RNG seed on reboot

[Jason A. Donenfeld]

On 9/26/22, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Jason,
>
> On Fri, Sep 23, 2022 at 7:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Rather than rebooting into a system with no entropy, regenerate the RNG
>> seed before rebooting, so that the new system has a fresh seed.
>>
>> Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> Thanks for your patch!
>
> I still doubt this is actually guaranteed to work, as the memory containing
> the bootinfo might be overwritten during normal operation.

Thus far, I'm unable to make it *not* work. So this seems like
actually an improvement?

Jason

[PATCH v3 3/3] m68k: rework BI_VIRT_RNG_SEED as BI_RNG_SEED

[Jason A. Donenfeld]

This is useful on !virt platforms for kexec, so change things from
BI_VIRT_RNG_SEED to be BI_RNG_SEED, and simply remove BI_VIRT_RNG_SEED
because it only ever lasted one release, and nothing is broken by not
having it. At the same time, keep a comment noting that it's been
removed, so that ID isn't reused.

Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/m68k/include/asm/bootinfo.h           |  2 ++
 arch/m68k/include/uapi/asm/bootinfo-virt.h |  9 ++-------
 arch/m68k/include/uapi/asm/bootinfo.h      |  8 +++++++-
 arch/m68k/kernel/process.c                 | 10 ++++++++++
 arch/m68k/kernel/setup_mm.c                | 14 ++++++++++++++
 arch/m68k/virt/config.c                    | 21 ---------------------
 6 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/arch/m68k/include/asm/bootinfo.h b/arch/m68k/include/asm/bootinfo.h
index 81c91af8ec6c..71103530839a 100644
--- a/arch/m68k/include/asm/bootinfo.h
+++ b/arch/m68k/include/asm/bootinfo.h
@@ -28,6 +28,8 @@ void process_uboot_commandline(char *commandp, int size);
 static inline void process_uboot_commandline(char *commandp, int size) {}
 #endif
 
+extern const struct bi_record *rng_seed_record;
+
 #endif /* __ASSEMBLY__ */
 
 
diff --git a/arch/m68k/include/uapi/asm/bootinfo-virt.h b/arch/m68k/include/uapi/asm/bootinfo-virt.h
index b091ee9b06e0..4032a14cc5c2 100644
--- a/arch/m68k/include/uapi/asm/bootinfo-virt.h
+++ b/arch/m68k/include/uapi/asm/bootinfo-virt.h
@@ -13,13 +13,8 @@
 #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 ]
- */
-#define BI_VIRT_RNG_SEED	0x8006
+/* No longer used -- replaced with BI_RNG_SEED -- but don't reuse this index:
+ * 	#define BI_VIRT_RNG_SEED	0x8006 */
 
 #define VIRT_BOOTI_VERSION	MK_BI_VERSION(2, 0)
 
diff --git a/arch/m68k/include/uapi/asm/bootinfo.h b/arch/m68k/include/uapi/asm/bootinfo.h
index 95ecf3ae4c49..387f2a61e908 100644
--- a/arch/m68k/include/uapi/asm/bootinfo.h
+++ b/arch/m68k/include/uapi/asm/bootinfo.h
@@ -64,7 +64,13 @@ struct mem_info {
 					/* (struct mem_info) */
 #define BI_COMMAND_LINE		0x0007	/* kernel command line parameters */
 					/* (string) */
-
+/*
+ * 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 ]
+ */
+#define BI_RNG_SEED		0x0008
 
     /*
      *  Linux/m68k Architectures (BI_MACHTYPE)
diff --git a/arch/m68k/kernel/process.c b/arch/m68k/kernel/process.c
index 2cb4a61bcfac..f40cc6f47f09 100644
--- a/arch/m68k/kernel/process.c
+++ b/arch/m68k/kernel/process.c
@@ -29,6 +29,7 @@
 #include <linux/reboot.h>
 #include <linux/init_task.h>
 #include <linux/mqueue.h>
+#include <linux/random.h>
 #include <linux/rcupdate.h>
 #include <linux/syscalls.h>
 #include <linux/uaccess.h>
@@ -36,6 +37,7 @@
 #include <asm/traps.h>
 #include <asm/machdep.h>
 #include <asm/setup.h>
+#include <asm/bootinfo.h>
 
 
 asmlinkage void ret_from_fork(void);
@@ -51,8 +53,16 @@ void arch_cpu_idle(void)
 #endif
 }
 
+const struct bi_record *rng_seed_record;
+
 void machine_restart(char * __unused)
 {
+	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 = cpu_to_be16(len);
+	}
+
 	if (mach_reset)
 		mach_reset();
 	for (;;);
diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
index 7e7ef67cff8b..eacf734bea0e 100644
--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/nvram.h>
 #include <linux/initrd.h>
+#include <linux/random.h>
 
 #include <asm/bootinfo.h>
 #include <asm/byteorder.h>
@@ -151,6 +152,19 @@ static void __init m68k_parse_bootinfo(const struct bi_record *record)
 				sizeof(m68k_command_line));
 			break;
 
+		case BI_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);
+			/* Store a reference to be filled in on reboot. */
+			rng_seed_record = record;
+			break;
+		}
+
 		default:
 			if (MACH_IS_AMIGA)
 				unknown = amiga_parse_bootinfo(record);
diff --git a/arch/m68k/virt/config.c b/arch/m68k/virt/config.c
index d4627840e35b..632ba200ad42 100644
--- a/arch/m68k/virt/config.c
+++ b/arch/m68k/virt/config.c
@@ -2,7 +2,6 @@
 
 #include <linux/reboot.h>
 #include <linux/serial_core.h>
-#include <linux/random.h>
 #include <clocksource/timer-goldfish.h>
 
 #include <asm/bootinfo.h>
@@ -45,18 +44,10 @@ static void virt_halt(void)
 		;
 }
 
-static const struct bi_record *rng_seed_record;
-
 static void virt_reset(void)
 {
 	void __iomem *base = (void __iomem *)virt_bi_data.ctrl.mmio;
 
-	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 = cpu_to_be16(len);
-	}
-
 	iowrite32be(CMD_RESET, base + VIRT_CTRL_REG_CMD);
 	local_irq_disable();
 	while (1)
@@ -101,18 +92,6 @@ 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);
-		/* Store a reference to be filled in on reboot. */
-		rng_seed_record = record;
-		break;
-	}
 	default:
 		unknown = 1;
 		break;
-- 
2.37.3

Re: [PATCH v3 3/3] m68k: rework BI_VIRT_RNG_SEED as BI_RNG_SEED

[Geert Uytterhoeven]

Hi Jason,

On Fri, Sep 23, 2022 at 7:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> This is useful on !virt platforms for kexec, so change things from
> BI_VIRT_RNG_SEED to be BI_RNG_SEED, and simply remove BI_VIRT_RNG_SEED
> because it only ever lasted one release, and nothing is broken by not
> having it. At the same time, keep a comment noting that it's been
> removed, so that ID isn't reused.
>
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

The approach LGTM, but given my doubts on [PATCH v3 2/3], I think this
patch should be moved up, to not depend on [2/3].

> --- a/arch/m68k/include/uapi/asm/bootinfo-virt.h
> +++ b/arch/m68k/include/uapi/asm/bootinfo-virt.h
> @@ -13,13 +13,8 @@
>  #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 ]
> - */
> -#define BI_VIRT_RNG_SEED       0x8006
> +/* No longer used -- replaced with BI_RNG_SEED -- but don't reuse this index:
> + *     #define BI_VIRT_RNG_SEED        0x8006 */

Please drop the space before TAB.

> --- a/arch/m68k/include/uapi/asm/bootinfo.h
> +++ b/arch/m68k/include/uapi/asm/bootinfo.h
> @@ -64,7 +64,13 @@ struct mem_info {
>                                         /* (struct mem_info) */
>  #define BI_COMMAND_LINE                0x0007  /* kernel command line parameters */
>                                         /* (string) */
> -
> +/*
> + * 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 ]

While experimenting, I noticed this must actually be a 4-byte alignment,
as data[] is an integral multiple of __be32 words:

    struct bi_record {
            __be16 tag;                     /* tag ID */
            __be16 size;                    /* size of record (in bytes) */
            __be32 data[];                  /* data */
    };

m68kboot[1] and kexec-boot[2] do implement the correct rounding,
but qemu's BOOTINFOSTR()[3] and BOOTINFODATA()[4] do not.
Sorry for missing that before, I guess I was misled by the existing
bug in BOOTINFOSTR().

Consequently, both kexec-tools[5] and the m68k-bootinfo tool[6] cannot
parse /proc/bootinfo if the size is not a multiple of 4, which can
easily be triggered by changing the command line of the m68k virt
machine.

> + */
> +#define BI_RNG_SEED            0x0008
>
>      /*
>       *  Linux/m68k Architectures (BI_MACHTYPE)

[1] https://github.com/geertu/m68kboot/blob/master/common/bootinf.c#L171
[2] https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/arch/m68k/bootinfo.c#n60
[3] https://git.qemu.org/?p=qemu.git;a=blob;f=hw/m68k/bootinfo.h;h=bd8b212fd35ce216917e07eb67783b5b69f1e11b;hb=HEAD#l57
[4] https://git.qemu.org/?p=qemu.git;a=blob;f=hw/m68k/bootinfo.h;h=bd8b212fd35ce216917e07eb67783b5b69f1e11b;hb=HEAD#l73
[5] https://git.kernel.org/pub/scm/utils/kernel/kexec/kexec-tools.git/tree/kexec/arch/m68k/bootinfo.c#n106
[6] https://github.com/geertu/m68k-bootinfo/blob/master/m68k-bootinfo.c#L466

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v3 3/3] m68k: rework BI_VIRT_RNG_SEED as BI_RNG_SEED

[Jason A. Donenfeld]

Hi Geert,

On Mon, Sep 26, 2022 at 12:27:13PM +0200, Geert Uytterhoeven wrote:
> The approach LGTM, but given my doubts on [PATCH v3 2/3], I think this
> patch should be moved up, to not depend on [2/3].

I'll do that. But also, I'm not sure I yet agree with you about 2/3, so
we should keep discussing that. (I'll still move it to 3/3 though.)

> > -#define BI_VIRT_RNG_SEED       0x8006
> > +/* No longer used -- replaced with BI_RNG_SEED -- but don't reuse this index:
> > + *     #define BI_VIRT_RNG_SEED        0x8006 */
> 
> Please drop the space before TAB.

Ack.

> 
> > --- a/arch/m68k/include/uapi/asm/bootinfo.h
> > +++ b/arch/m68k/include/uapi/asm/bootinfo.h
> > @@ -64,7 +64,13 @@ struct mem_info {
> >                                         /* (struct mem_info) */
> >  #define BI_COMMAND_LINE                0x0007  /* kernel command line parameters */
> >                                         /* (string) */
> > -
> > +/*
> > + * 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 ]
> 
> While experimenting, I noticed this must actually be a 4-byte alignment,
> as data[] is an integral multiple of __be32 words:
> 
>     struct bi_record {
>             __be16 tag;                     /* tag ID */
>             __be16 size;                    /* size of record (in bytes) */
>             __be32 data[];                  /* data */
>     };
> 
> m68kboot[1] and kexec-boot[2] do implement the correct rounding,
> but qemu's BOOTINFOSTR()[3] and BOOTINFODATA()[4] do not.
> Sorry for missing that before, I guess I was misled by the existing
> bug in BOOTINFOSTR().
> 
> Consequently, both kexec-tools[5] and the m68k-bootinfo tool[6] cannot
> parse /proc/bootinfo if the size is not a multiple of 4, which can
> easily be triggered by changing the command line of the m68k virt
> machine.

In my brief experience in working with this structure, I've just made
sure that data[] is casted to (void*) or (u8*) or something, and then
everything is fine. But the tools you've linked to expect 4 byte
alignment. So, it sounds like what you're saying is that while we're at
it with fixing the constant, I should also make sure seed data preserves
4 byte alignment, and update the comment too? That's easy enough. I'll
do that for v+1.

Jason

Re: [PATCH v3 3/3] m68k: rework BI_VIRT_RNG_SEED as BI_RNG_SEED

[Jason A. Donenfeld]

Okay v4 on its way now. Also updating the qemu patch to fix the bug you found.

Re: [PATCH v3 1/3] m68k: process bootinfo records before saving them

[Geert Uytterhoeven]

On Fri, Sep 23, 2022 at 7:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> The RNG seed boot record is memzeroed after processing, in order to
> preserve forward secrecy. By saving the bootinfo for procfs prior to
> that, forward secrecy is violated, since it becomes possible to recover
> past states. So, save the bootinfo block only after first processing
> them.
>
> Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v3 1/3] m68k: process bootinfo records before saving them

[Jason A. Donenfeld]

Hi Geert,

On 9/26/22, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Sep 23, 2022 at 7:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> The RNG seed boot record is memzeroed after processing, in order to
>> preserve forward secrecy. By saving the bootinfo for procfs prior to
>> that, forward secrecy is violated, since it becomes possible to recover
>> past states. So, save the bootinfo block only after first processing
>> them.
>>
>> Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks for the review.

Did you intend to take these 3 patches through your m68k tree, or did
you want me to take them for some reason instead?

Jason

Re: [PATCH v3 1/3] m68k: process bootinfo records before saving them

[Geert Uytterhoeven]

Hi Jason,

On Mon, Sep 26, 2022 at 12:06 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On 9/26/22, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Sep 23, 2022 at 7:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >> The RNG seed boot record is memzeroed after processing, in order to
> >> preserve forward secrecy. By saving the bootinfo for procfs prior to
> >> that, forward secrecy is violated, since it becomes possible to recover
> >> past states. So, save the bootinfo block only after first processing
> >> them.
> >>
> >> Fixes: a1ee38ab1a75 ("m68k: virt: Use RNG seed from bootinfo block")
> >> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Thanks for the review.
>
> Did you intend to take these 3 patches through your m68k tree, or did
> you want me to take them for some reason instead?

This is all TBD, see the comments I'm writing on the other patches ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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