On Sat, 20 Aug 2022 at 00:23, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Thu, Aug 18, 2022 at 06:12:55PM +0200, Ard Biesheuvel wrote:
> > 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?
>
> 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))
>
I think we should add this to drivers/firmware/efi/libstub/Makefile
Daniel, could you please verify whether that fixes your issue as well? Thanks.
