On Mon, 22 Aug 2022 at 08:24, Daniel Marth
<daniel.marth@xxxxxxxxxxxxxxxxx> wrote:
>
> On 20/08/2022 19:58, Ard Biesheuvel wrote:
> > 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.
>
> The suggested modification of KBUILD_CFLAGS solves my issue.
Thanks for the confirmation. I will send it out as a proper patch.
