[PATCH] efi/libstub: Disable RNG structure randomization

[PATCH] efi/libstub: Disable RNG structure randomization

[Daniel Marth]

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>
---
 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;
-- 
2.37.2

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

[Ard Biesheuvel]

(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/

Kees?

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

[Kees Cook]

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.

-Kees

-- 
Kees Cook

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

[Ard Biesheuvel]

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?

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

[Kees Cook]

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

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

[Ard Biesheuvel]

On Sat, 20 Aug 2022 at 00:23, Kees Cook <keescook@chromium.org> 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@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))
>

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.

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

[Daniel Marth]

On 20/08/2022 19:58, Ard Biesheuvel wrote:
> On Sat, 20 Aug 2022 at 00:23, Kees Cook <keescook@chromium.org> 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@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))
>>
> 
> 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.

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

[Ard Biesheuvel]

On Mon, 22 Aug 2022 at 08:24, Daniel Marth
<daniel.marth@inso.tuwien.ac.at> wrote:
>
> On 20/08/2022 19:58, Ard Biesheuvel wrote:
> > On Sat, 20 Aug 2022 at 00:23, Kees Cook <keescook@chromium.org> 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@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))
> >>
> >
> > 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.