Re: [PATCH] efi/libstub: Disable RNG structure randomization

[Kees Cook <keescook@chromium.org>]

On Thu, Aug 18, 2022 at 06:12:55PM +0200, Ard Biesheuvel wrote:
> On Thu, 18 Aug 2022 at 18:02, Kees Cook <keescook@chromium.org> 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@inso.tuwien.ac.at> 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@inso.tuwien.ac.at>
> > >
> > > 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?

I don't know if that got looked at -- there wasn't a place where such a
mixture was needed before. Everywhere else just marks individual
structs. Places like vDSO do stuff like this for their Makefile:

KBUILD_CFLAGS := $(filter-out $(RANDSTRUCT_CFLAGS),$(KBUILD_CFLAGS))

-- 
Kees Cook