From 1dd055f8e394d5115e3a13c9c704e6cc7e5c8da7 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Fri, 30 Jun 2023 14:48:17 -0400 Subject: [PATCH] do the rest of the loader protocol --- BUILDING | 9 -- Make.defaults | 4 - globals.c | 1 - include/loader-proto.h | 3 +- loader-proto.c | 196 ++++++++++++++--------------------------- shim.c | 4 - 6 files changed, 66 insertions(+), 151 deletions(-) diff --git a/BUILDING b/BUILDING index 17cd98d32..c676b83b4 100644 --- a/BUILDING +++ b/BUILDING @@ -37,15 +37,6 @@ Variables you could set to customize the build: debugger only on the development branch and not the OS you need to boot to scp in a new development build. Likewise, we look for SHIM_DEVEL_VERBOSE rather than SHIM_VERBOSE. -- DISABLE_EBS_PROTECTION - On systems where a second stage bootloader is not used, and the Linux - Kernel is embedded in the same EFI image as shim and booted directly - from shim, shim's ExitBootServices() hook can cause problems as the - kernel never calls the shim's verification protocol. In this case - calling the shim verification protocol is unnecessary and redundant as - shim has already verified the kernel when shim loaded the kernel as the - second stage loader. In such a case, and only in this case, you should - use DISABLE_EBS_PROTECTION=y to build. - DISABLE_REMOVABLE_LOAD_OPTIONS Do not parse load options when invoked as boot*.efi. This prevents boot failures because of unexpected data in boot entries automatically generated diff --git a/Make.defaults b/Make.defaults index e75cd3cdd..85160f6dd 100644 --- a/Make.defaults +++ b/Make.defaults @@ -149,10 +149,6 @@ ifneq ($(origin REQUIRE_TPM), undefined) DEFINES += -DREQUIRE_TPM endif -ifneq ($(origin DISABLE_EBS_PROTECTION), undefined) - DEFINES += -DDISABLE_EBS_PROTECTION -endif - ifneq ($(origin DISABLE_REMOVABLE_LOAD_OPTIONS), undefined) DEFINES += -DDISABLE_REMOVABLE_LOAD_OPTIONS endif diff --git a/globals.c b/globals.c index 91916e988..d574df160 100644 --- a/globals.c +++ b/globals.c @@ -24,7 +24,6 @@ UINT8 *build_cert; * indicator of how an image has been verified */ verification_method_t verification_method; -int loader_is_participating; SHIM_IMAGE_LOADER shim_image_loader_interface; diff --git a/include/loader-proto.h b/include/loader-proto.h index d3afa2f5b..db8e670e6 100644 --- a/include/loader-proto.h +++ b/include/loader-proto.h @@ -16,7 +16,6 @@ typedef enum { } verification_method_t; extern verification_method_t verification_method; -extern int loader_is_participating; extern void hook_system_services(EFI_SYSTEM_TABLE *local_systab); extern void unhook_system_services(void); @@ -27,6 +26,8 @@ extern void unhook_exit(void); typedef struct _SHIM_IMAGE_LOADER { EFI_IMAGE_LOAD LoadImage; EFI_IMAGE_START StartImage; + EFI_EXIT Exit; + EFI_IMAGE_UNLOAD UnloadImage; } SHIM_IMAGE_LOADER; extern SHIM_IMAGE_LOADER shim_image_loader_interface; diff --git a/loader-proto.c b/loader-proto.c index a61a91db5..5c6b5330d 100644 --- a/loader-proto.c +++ b/loader-proto.c @@ -32,12 +32,8 @@ get_active_systab(void) static typeof(systab->BootServices->LoadImage) system_load_image; static typeof(systab->BootServices->StartImage) system_start_image; +static typeof(systab->BootServices->UnloadImage) system_unload_image; static typeof(systab->BootServices->Exit) system_exit; -#if !defined(DISABLE_EBS_PROTECTION) -static typeof(systab->BootServices->ExitBootServices) system_exit_boot_services; -#endif /* !defined(DISABLE_EBS_PROTECTION) */ - -static EFI_HANDLE last_loaded_image; void unhook_system_services(void) @@ -47,9 +43,8 @@ unhook_system_services(void) systab->BootServices->LoadImage = system_load_image; systab->BootServices->StartImage = system_start_image; -#if !defined(DISABLE_EBS_PROTECTION) - systab->BootServices->ExitBootServices = system_exit_boot_services; -#endif /* !defined(DISABLE_EBS_PROTECTION) */ + systab->BootServices->Exit = system_exit; + systab->BootServices->UnloadImage = system_unload_image; BS = systab->BootServices; } @@ -108,6 +103,14 @@ shim_start_image(IN EFI_HANDLE ImageHandle, OUT UINTN *ExitDataSize, efi_status = BS->HandleProtocol(ImageHandle, &SHIM_LOADED_IMAGE_GUID, (void **)&image); + + /* + * This image didn't come from shim_load_image(), so it must have come + * from something before shim was involved. + */ + if (efi_status == EFI_UNSUPPORTED) + return system_start_image(ImageHandle, ExitDataSize, ExitData); + if (EFI_ERROR(efi_status) || image->started) return EFI_INVALID_PARAMETER; @@ -139,131 +142,54 @@ shim_start_image(IN EFI_HANDLE ImageHandle, OUT UINTN *ExitDataSize, } static EFI_STATUS EFIAPI -load_image(BOOLEAN BootPolicy, EFI_HANDLE ParentImageHandle, - EFI_DEVICE_PATH *DevicePath, VOID *SourceBuffer, - UINTN SourceSize, EFI_HANDLE *ImageHandle) +shim_unload_image(EFI_HANDLE ImageHandle) { + SHIM_LOADED_IMAGE *image; EFI_STATUS efi_status; - unhook_system_services(); - efi_status = BS->LoadImage(BootPolicy, ParentImageHandle, DevicePath, - SourceBuffer, SourceSize, ImageHandle); - hook_system_services(systab); - if (EFI_ERROR(efi_status)) - last_loaded_image = NULL; - else - last_loaded_image = *ImageHandle; - return efi_status; -} + efi_status = BS->HandleProtocol(ImageHandle, &SHIM_LOADED_IMAGE_GUID, + (void **)&image); -static EFI_STATUS EFIAPI -replacement_start_image(EFI_HANDLE image_handle, UINTN *exit_data_size, CHAR16 **exit_data) -{ - EFI_STATUS efi_status; - unhook_system_services(); - - if (image_handle == last_loaded_image) { - UINT8 retain_protocol = 0; - UINTN retain_protocol_size = sizeof(retain_protocol); - UINT32 retain_protocol_attrs = 0; - - loader_is_participating = 1; - - /* If a boot component asks us, keep our protocol around - it will be used to - * validate further PE payloads (e.g.: by the UKI stub, before the kernel is booted). - * But also check that the variable was set by a boot component, to ensure that - * nobody at runtime can attempt to change shim's behaviour. */ - efi_status = RT->GetVariable(SHIM_RETAIN_PROTOCOL_VAR_NAME, - &SHIM_LOCK_GUID, - &retain_protocol_attrs, - &retain_protocol_size, - &retain_protocol); - if (EFI_ERROR(efi_status) || - (retain_protocol_attrs & EFI_VARIABLE_NON_VOLATILE) || - !(retain_protocol_attrs & EFI_VARIABLE_BOOTSERVICE_ACCESS) || - retain_protocol_size != sizeof(retain_protocol) || - retain_protocol == 0) - uninstall_shim_protocols(); - } - efi_status = BS->StartImage(image_handle, exit_data_size, exit_data); - if (EFI_ERROR(efi_status)) { - if (image_handle == last_loaded_image) { - EFI_STATUS efi_status2 = install_shim_protocols(); - - if (EFI_ERROR(efi_status2)) { - console_print(L"Something has gone seriously wrong: %r\n", - efi_status2); - console_print(L"shim cannot continue, sorry.\n"); - usleep(5000000); - RT->ResetSystem(EfiResetShutdown, - EFI_SECURITY_VIOLATION, - 0, NULL); - } - } - hook_system_services(systab); - loader_is_participating = 0; - } - return efi_status; -} + if (efi_status == EFI_UNSUPPORTED) + return system_unload_image(ImageHandle); -#if !defined(DISABLE_EBS_PROTECTION) -static EFI_STATUS EFIAPI -exit_boot_services(EFI_HANDLE image_key, UINTN map_key) -{ - if (loader_is_participating || - verification_method == VERIFIED_BY_HASH) { - unhook_system_services(); - EFI_STATUS efi_status; - efi_status = BS->ExitBootServices(image_key, map_key); - if (EFI_ERROR(efi_status)) - hook_system_services(systab); - return efi_status; - } + BS->FreePages(image->alloc_address, image->alloc_pages); + FreePool(image); - console_print(L"Bootloader has not verified loaded image.\n"); - console_print(L"System is compromised. halting.\n"); - usleep(5000000); - RT->ResetSystem(EfiResetShutdown, EFI_SECURITY_VIOLATION, 0, NULL); - return EFI_SECURITY_VIOLATION; + return EFI_SUCCESS; } -#endif /* !defined(DISABLE_EBS_PROTECTION) */ static EFI_STATUS EFIAPI -do_exit(EFI_HANDLE ImageHandle, EFI_STATUS ExitStatus, - UINTN ExitDataSize, CHAR16 *ExitData) +shim_exit(EFI_HANDLE ImageHandle, + EFI_STATUS ExitStatus, + UINTN ExitDataSize, + CHAR16 *ExitData) { EFI_STATUS efi_status; SHIM_LOADED_IMAGE *image; efi_status = BS->HandleProtocol(ImageHandle, &SHIM_LOADED_IMAGE_GUID, (void **)&image); - if (!EFI_ERROR(efi_status)) { - image->exit_status = ExitStatus; - image->exit_data_size = ExitDataSize; - image->exit_data = ExitData; - longjmp(image->longjmp_buf, 1); + /* + * If this happens, something above us on the stack of running + * applications called Exit(), and we're getting aborted along with + * it. + */ + if (efi_status == EFI_UNSUPPORTED) { + shim_fini(); + return system_exit(ImageHandle, ExitStatus, ExitDataSize, + ExitData); } - shim_fini(); - - restore_loaded_image(); + if (EFI_ERROR(efi_status)) + return efi_status; - efi_status = BS->Exit(ImageHandle, ExitStatus, - ExitDataSize, ExitData); - if (EFI_ERROR(efi_status)) { - EFI_STATUS efi_status2 = shim_init(); + image->exit_status = ExitStatus; + image->exit_data_size = ExitDataSize; + image->exit_data = ExitData; - if (EFI_ERROR(efi_status2)) { - console_print(L"Something has gone seriously wrong: %r\n", - efi_status2); - console_print(L"shim cannot continue, sorry.\n"); - usleep(5000000); - RT->ResetSystem(EfiResetShutdown, - EFI_SECURITY_VIOLATION, 0, NULL); - } - } - return efi_status; + longjmp(image->longjmp_buf, 1); } void @@ -271,6 +197,8 @@ init_image_loader(void) { shim_image_loader_interface.LoadImage = shim_load_image; shim_image_loader_interface.StartImage = shim_start_image; + shim_image_loader_interface.Exit = shim_exit; + shim_image_loader_interface.UnloadImage = shim_unload_image; } void @@ -281,27 +209,31 @@ hook_system_services(EFI_SYSTEM_TABLE *local_systab) /* We need to hook various calls to make this work... */ - /* We need LoadImage() hooked so that fallback.c can load shim - * without having to fake LoadImage as well. This allows it - * to call the system LoadImage(), and have us track the output - * and mark loader_is_participating in replacement_start_image. This - * means anything added by fallback has to be verified by the system - * db, which we want to preserve anyway, since that's all launching - * through BDS gives us. */ + /* + * We need LoadImage() hooked so that we can guarantee everything is + * verified. + */ system_load_image = systab->BootServices->LoadImage; - systab->BootServices->LoadImage = load_image; + systab->BootServices->LoadImage = shim_load_image; - /* we need StartImage() so that we can allow chain booting to an - * image trusted by the firmware */ + /* + * We need StartImage() hooked because the system's StartImage() + * doesn't know about our structure layout. + */ system_start_image = systab->BootServices->StartImage; - systab->BootServices->StartImage = replacement_start_image; - -#if !defined(DISABLE_EBS_PROTECTION) - /* we need to hook ExitBootServices() so a) we can enforce the policy - * and b) we can unwrap when we're done. */ - system_exit_boot_services = systab->BootServices->ExitBootServices; - systab->BootServices->ExitBootServices = exit_boot_services; -#endif /* defined(DISABLE_EBS_PROTECTION) */ + systab->BootServices->StartImage = shim_start_image; + + /* + * We need Exit() hooked so that we make sure to use the right jmp_buf + * when an application calls Exit(), but that happens in a separate + * function. + */ + + /* + * We need UnloadImage() to match our LoadImage() + */ + system_unload_image = systab->BootServices->UnloadImage; + systab->BootServices->UnloadImage = shim_unload_image; } void @@ -321,5 +253,5 @@ hook_exit(EFI_SYSTEM_TABLE *local_systab) * bootloader and still e.g. start a new one or run an internal * shell. */ system_exit = systab->BootServices->Exit; - systab->BootServices->Exit = do_exit; + systab->BootServices->Exit = shim_exit; } diff --git a/shim.c b/shim.c index 5f7c1ccaf..85805b76e 100644 --- a/shim.c +++ b/shim.c @@ -994,7 +994,6 @@ EFI_STATUS shim_verify (void *buffer, UINT32 size) if ((INT32)size < 0) return EFI_INVALID_PARAMETER; - loader_is_participating = 1; in_protocol = 1; efi_status = read_header(buffer, size, &context); @@ -1215,8 +1214,6 @@ EFI_STATUS start_image(EFI_HANDLE image_handle, CHAR16 *ImagePath) goto restore; } - loader_is_participating = 0; - /* * The binary is trusted and relocated. Run it */ @@ -1728,7 +1725,6 @@ shim_init(void) * validation of the next image. */ hook_system_services(systab); - loader_is_participating = 0; } }