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>
In-Reply-To: <CAHmME9pAsY4M=V0o4QLrrQiXwCvtiEO9FApBibXRiG41h-AgVA@mail.gmail.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Fri, 23 Sep 2022 14:23:41 +0200
Message-ID: <CAMuHMdUSF=ARvKTP33psHNWEqvSeUUDuWLRsUUA7LHa+12agng@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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Jason,

On Fri, Sep 23, 2022 at 1:53 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Fri, Sep 23, 2022 at 1:30 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > +static struct bi_record *rng_seed_record;
> >
> > This can be const...
> > >                 memzero_explicit((void *)data, len + 2);
> > > +                /* Store a reference to be filled in on reboot. */
> > > +               rng_seed_record = (void *)record;
> >
> > ... so this cast can be dropped.
>
> Will do.
>
> >
> > > +
> > >  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 = len;

Storing the length should use the proper cpu_to_be16 accessor.

> > Wouldn't it be simpler to just use the existing length?
> >
> >     if (rnd_seed_record) {
> >            u16 len = be16_to_cpup(data);
> >            get_random_bytes((u8 *)rng_seed_record->data + 2, len);
> >     }
>
> No, that would not work. len is 0 there, since we zero out the bytes
> after use for forward secrecy, and we zero out the length, so that we
> don't wind up feeding it zeros.

You're right. I misread the location of the "+ 2" in the clearing code.

> > However, I have my doubts this will actually work. Was this tested?
> > The bootinfo is passed from userspace, usually by reading
> > /proc/bootinfo, and adapting it where needed.
> > So I think you should implement this in kexec-tools instead.
>
> Yes, this was tested. This is to handle the reboot case, just as the
> commit subject says. Specifically, calling `reboot(RB_AUTOBOOT);`.

OK.

> It does *not* handle kexec. For that, indeed, kexec-tools needs to be
> augmented, but that's a separate patch that doesn't need to interact
> with this one.
>
> 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.

> I'll send a v2 with that const fix.

OK, thanks!

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