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.
