Re: [PATCH 2/2] arm64: efi: kaslr: Fix boot failure if efi_random_alloc() fails

[Ard Biesheuvel <ardb@kernel.org>]

On Tue, 20 Jul 2021 at 16:04, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2021-07-20 at 15:48 +0200, Ard Biesheuvel wrote:
> >
> > You are replacing min_kimg_align() with MIN_KIMG_ALIGN in a place
> > where it could return either value: efi_nokaslr will be false by
> > default on relocatable kernels
>
> Not exactly:
>
> drivers/firmware/efi/libstub/efi-stub-helper.c:bool efi_nokaslr =
> !IS_ENABLED(CONFIG_RANDOMIZE_BASE);
>
> So if CONFIG_RANDOMIZE_BASE is off (KASLR disabled in the config),
> efi_nokaslr is true.
>
> If CONFIG_RANDOMIZE_BASE is on, then it depends on the command line
> (and the availability of the RNG protocol).
>
> None of this depends on CONFIG_RELOCATABLE which is indeed not entirely
> orthogonal, but not particularily relevant in how the code is written
> today.
>
> > , in which case min_kimg_align() will
> > return EFI_KIMG_ALIGN, unless you specifically request KASLR to be
> > disabled.
>
> Nope. See above. It will only be EFI_KIMG_ALIGN if
> CONFIG_RANDOMIZE_BASE is on and KASLR isn't otherwise disabled.
>
> > The result is that relocatable kernels that would not require to be
> > moved will now be moved to a 2 MB aligned offset before booting them.
> >
> > Similarly for the efi_allocate_pages_aligned() call: that call would
> > only request 64k alignment before on a relocatable kernel if booting
> > without randomization.
>
> I'm not sure I'm following you here. If you look at the changelog for
> commit 7c116db24d94, it pretty clearly says:
>
> "Adjust the EFI stub for arm64 so that the minimum Image alignment is
> 2MB unless KASLR is in use."
>
> Which is also pretty much what is spelled in the comment
> above min_kimg_align() (which I moved but kept in my patch).
>
> Basically, what you describe is what the code used to do afaik, but not
> what it does since 7c116db24d94.
>
> The current code (prior) to my patch is pretty clear, it uses 64k
> alignment if KASLR is on, otherwise 2MB. So the big if (status !=
> EFI_SUCCESS) statement with the alignment check & relocation is all
> only meant to be used in the !KASLR case, which is always going to want
> 2MB (again based on the code as written today).
>
> My patch simply ensures that this is also true when KASLR fails to
> randomize the kernel address.
>

Fair enough.

The history here is that passing nokaslr on the command line would
force 2M alignment even if KASLR was not enabled to begin with,
without affecting the alignment policy of KASLR capable kernels if
KASLR was not explicitly disabled, but not available on the platform.

I realize now that my commit d32de9130f6c7 has interfered with this:
efi_nokaslr is set to true there so the later code does not complain
about the EFI_RNG_PROTOCOL being unavailable, but it has the side
effect of affecting the alignment policy in the remainder of the
function.

So what I would prefer here is to capture efi_nokaslr at entry, and
use that to decide the alignment. That way, efi_nokaslr can be set to
true without affecting the subsequent allocation logic.