On Thu, 18 Aug 2022 at 18:02, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Thu, Aug 18, 2022 at 09:10:23AM +0200, Ard Biesheuvel wrote:
> > (cc Kees)
> >
> > On Thu, 18 Aug 2022 at 08:58, Daniel Marth
> > <daniel.marth@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > Randstruct by default randomizes structures that consist entirely of
> > > function pointers, even if they are not explicitly labeled for
> > > randomization. efi_rng_protocol contains an anonymous structure that is
> > > affected by this implicit selection process. Randomization of this
> > > structure causes a data layout inconsistency between the kernel and the
> > > EFI. In this scenario the Arm64 boot process fails with the following
> > > output:
> > > EFI stub: Booting Linux Kernel...
> > > EFI stub: ERROR: efi_get_random_bytes() failed (0x8000000000000002)
> > > EFI stub: Using DTB from configuration table
> > > EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
> > > Synchronous Exception at 0x0000000081310C90
> > > Synchronous Exception at 0x0000000081310C90
> > >
> > > efi_get_random_bytes() fails in handle_kernel_image (arm64-stub.c)
> > > because it uses an incorrect structure layout for efi_call_proto. Add
> > > the __no_randomize_layout annotation to the anonymous structure within
> > > efi_rng_protocol to prevent its randomization and resolve this issue.
> > >
> > > This patch was tested for the Arm64 architecture using QEMU. In
> > > addition to the current next branch of this subsystem, also minor
> > > versions 4.16 to 5.1, 5.5 and 5.6 were tested successfully with a
> > > (backported) version of this patch.
> > >
> > > Signed-off-by: Daniel Marth <daniel.marth@xxxxxxxxxxxxxxxxx>
> >
> > Thanks for the patch.
> >
> > > ---
> > > drivers/firmware/efi/libstub/random.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
> > > index 24aa37535372..54fa980cf1af 100644
> > > --- a/drivers/firmware/efi/libstub/random.c
> > > +++ b/drivers/firmware/efi/libstub/random.c
> > > @@ -18,7 +18,7 @@ union efi_rng_protocol {
> > > efi_status_t (__efiapi *get_rng)(efi_rng_protocol_t *,
> > > efi_guid_t *, unsigned long,
> > > u8 *out);
> > > - };
> > > + } __no_randomize_layout;
> > > struct {
> > > u32 get_info;
> > > u32 get_rng;
> >
> > This may work around the problem, but I'd like to fix this more
> > thoroughly if we can. EFI protocols are not randomizable by nature, as
> > they are a contract between the firmware and the OS, so struct
> > randomization should just be disabled for the entire EFI stub, i.e.,
> > everything below libstub/
>
> So, yeah, any external interface that uses function pointer tables
> needs to be marked as not randomized. I think disabling randstruct for
> the entire subdirectory may run into a reverse problem, if anything gets
> used in there that is randomized by the rest of the kernel. I'm not clear
> where there boundaries are on that, though, so I leave it up to your
> judgement. IMO, it seems cleanest to just mark any all-function-pointer
> structs as __no_randomize_layout.
>
But there are *lots* of those, and this makes it a moving target as well.
The handover from EFI to the kernel proper passes very little state,
so turning it off in the stub should not be an issue afaict.
What would be even better is a pragma push/pop that disables it for
all type definitions in between, Did anyone ever look into that?
