[PATCH v4] fdt: automatically add /chosen/kaslr-seed if DM_RNG is enabled

[Simon Glass]

Hi,

On Thu, 30 May 2024 at 09:17, Tim Harvey <tharvey at gateworks.com> wrote:
>
> On Wed, May 29, 2024 at 6:39 PM Marek Vasut <marex at denx.de> wrote:
> >
> > On 5/29/24 7:05 PM, Simon Glass wrote:
> >
> > [...]
> >
> > >>>> that is not yet implemented as DM_RNG. We also skip this if
> > >>>> MEASURED_BOOT is enabled as in that case any modifications to the
> > >>>> dt will cause measured boot to fail (although there are many other
> > >>>> places the dt is altered).
> > >>>>
> > >>>> As this fdt node is added elsewhere create a library function and
> > >>>> use it to deduplicate code. We will provide a parameter to specify the
> > >>>> index of the rng device as well as a boolean to overwrite if present.
> > >>>>
> > >>>> For our automatic injection, we will use the first rng device and
> > >>>> not overwrite if already present with a non-zero value (which may
> > >>>> have been populated by an earlier boot stage). This way if a board
> > >>>> specific ft_board_setup() function wants to customize this behavior
> > >>>> it can call fdt_kaslrseed with a rng device index of its choosing and
> > >>>> set overwrite true.
> > >>>
> > >>> I suggest creating a sysinfo driver which can provide the RNG device.
> > >>
> > >> I'm not really clear what you mean but that seems out of scope for
> > >> this patch. If/when someone needs to extend the functionality of
> > >> specifying one of many RNG's perhaps they can follow that approach.
> > >
> > > The thing is, you are creating a way to specify the RNG which will
> > > presumably lead to different vendors all doing it a different way.
> > >
> > > Specifically, fdt_kaslrseed() takes a device number (would be better
> > > to take a struct udevice, BTW).
> > >
> > > Instead, I think you should define the standard way that the chosen
> > > RNG should be specified by the board. The sysinfo driver can provide a
> > > way to do this. Then boards can handle this themselves, using sysinfo,
> > > rather than directly calling a function in the fixup. After all, we
> > > want the fixup flow to be as generic as possible.
> >
> > I think this patch simply picks up the first RNG that is available,
> > pulls a number out of it, and uses that as the KASLR seed in DT. That's
> > what the implementation(s) seems to be doing so far, this patch is a
> > deduplication only, so maybe we should retain the original behavior
> > first, and do architectural changes in follow up patch(es) ?
>
> Marek,
>
> Thank you - I had a draft saying exactly the same thing that I hadn't
> sent out yet.
>
> This is an incremental improvement... for sure there are follow-on
> improvements that can be made and should be made but the sysinfo and
> policy creation are something I don't have time for at the moment and
> may not be the right person to implement that. I'm happy to accept
> something to add to my series though!
>
> So the question is, if we can agree that improvements can continue to
> be made later should I do one more revision that split this out into
> three patches and fixes the typo in the commit?

OK, well specifically, my objection is to the 'index' argument, since
someone will pass 1 to it and then we have a new API.

So if you can drop that and go back to the hard-coded 0, then I agree
this is just a refactoring and yes, this is fine.

Regards,
Simon