Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220921185208.3549140-1-Jason@zx2c4.com> <20220921185208.3549140-2-Jason@zx2c4.com>
 <CAMuHMdUuM85s1APoxRmXnw13hHHEGgo8Z9EKvpV6maaZPaVUfA@mail.gmail.com>
 <CAHmME9pAsY4M=V0o4QLrrQiXwCvtiEO9FApBibXRiG41h-AgVA@mail.gmail.com>
 <CAMuHMdUSF=ARvKTP33psHNWEqvSeUUDuWLRsUUA7LHa+12agng@mail.gmail.com> <CAHmME9ocjS11ugpKKwmosqb2HUBf4_N74a056=OOoj06yD6wmQ@mail.gmail.com>
In-Reply-To: <CAHmME9ocjS11ugpKKwmosqb2HUBf4_N74a056=OOoj06yD6wmQ@mail.gmail.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Fri, 23 Sep 2022 14:50:21 +0200
Message-ID: <CAMuHMdWruwCNqM3eLaaM9L+D+ZTw+t6z3O+rjNWTMiCQyj4_zQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] m68k: virt: generate new RNG seed on reboot
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     linux-m68k@lists.linux-m68k.org, linux-kernel@vger.kernel.org,
        Laurent Vivier <Laurent@vivier.eu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

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