| Message ID | 20220818065635.243057-1-daniel.marth@inso.tuwien.ac.at |
|---|---|
| State | New |
| Headers | show |
| Series | efi/libstub: Disable RNG structure randomization | expand |
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.
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.
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.
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;
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(-)