| Message ID | 44cb9567aa17d6255beadaa48defccd246b35669.1611273359.git.bobbyeshleman@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support Secure Boot for multiboot2 Xen | expand |
On 22.01.2021 01:51, Bobby Eshleman wrote: > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -244,9 +244,13 @@ __efi64_mb2_start: > jmp x86_32_switch > > .Lefi_multiboot2_proto: > - /* Zero EFI SystemTable and EFI ImageHandle addresses. */ > + /* > + * Zero EFI SystemTable, EFI ImageHandle and > + * dom0 kernel module struct addresses. > + */ > xor %esi,%esi > xor %edi,%edi > + xor %r14d, %r14d Nit: There's little point in having the d suffixes here and below, and the code would be slightly easier to read without. > /* Skip Multiboot2 information fixed part. */ > lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx > @@ -284,6 +288,15 @@ __efi64_mb2_start: > cmove MB2_efi64_ih(%rcx),%rdi > je .Lefi_mb2_next_tag > > + /* Get Dom0 kernel module struct address from Multiboot2 information. */ > + cmpl $MULTIBOOT2_TAG_TYPE_MODULE,MB2_tag_type(%rcx) Not: If elsewhere in the code additions you put blanks after the comma (which I appreciate), please do so here as well. > + jne .Lefi_mb2_end > + > + test %r14d, %r14d > + cmovz %ecx, %r14d So this doesn't truncate the address because higher up %ecx was loaded instead of %rcx. I realize that's not code you add, but it still strikes me as odd. Are there indeed guarantees that all of this will live below 4Gb? > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -3,6 +3,8 @@ > * is intended to be included by common/efi/boot.c _only_, and > * therefore can define arch specific global variables. > */ > +#include <xen/types.h> > +#include <xen/multiboot2.h> > #include <xen/vga.h> > #include <asm/e820.h> > #include <asm/edd.h> > @@ -762,6 +764,10 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, Isn't there a hunk missing up from here to add the new parameter to efi_multiboot2()? > gop = efi_get_gop(); > > + if ( dom0_kernel && dom0_kernel->mod_end > dom0_kernel->mod_start ) > + efi_shim_lock((VOID *)(unsigned long)dom0_kernel->mod_start, > + dom0_kernel->mod_end - dom0_kernel->mod_start); While somewhat unrelated to the change itself - how come the fields are all u32 (and hence you need to cast to unsigned long first)? There having been requests to allow for about 1Gb initrd images, I find it quite reasonable to expect that modules may not all fit below 4Gb. > --- a/xen/arch/x86/efi/stub.c > +++ b/xen/arch/x86/efi/stub.c > @@ -1,7 +1,9 @@ > +#include <xen/types.h> Please don't, even less so without honoring the alphabetical sorting. > #include <xen/efi.h> > #include <xen/errno.h> > #include <xen/init.h> > #include <xen/lib.h> > +#include <xen/multiboot2.h> > #include <asm/asm_defns.h> > #include <asm/efibind.h> > #include <asm/page.h> > @@ -29,7 +31,8 @@ asm ( > ); > > void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, > - EFI_SYSTEM_TABLE *SystemTable) > + EFI_SYSTEM_TABLE *SystemTable, > + multiboot2_tag_module_t *dom0_kernel) const? > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -133,6 +133,7 @@ static void efi_console_set_mode(void); > static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void); > static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, > UINTN cols, UINTN rows, UINTN depth); > +static void efi_shim_lock(const VOID *Buffer, UINT32 Size); > static void efi_tables(void); > static void setup_efi_pci(void); > static void efi_variables(void); > @@ -830,6 +831,17 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, > return gop_mode; > } > > +static void __init efi_shim_lock(const VOID *Buffer, UINT32 Size) Maybe better efi_shim_lock_verify()? > +{ > + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; > + EFI_SHIM_LOCK_PROTOCOL *shim_lock; > + EFI_STATUS status; > + > + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, (void **)&shim_lock)) && Nit: Overly long line. > + (status = shim_lock->Verify(Buffer, Size)) != EFI_SUCCESS ) > + PrintErrMesg(L"Dom0 kernel image could not be verified", status); I'm willing to let it be as is, but in principle this function is not Dom0-specific the way you've split it out. _If_ you leave it this way, perhaps (on top of the suggestion above) perhaps better name it efi_shim_lock_verify_dom0()? Jan
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index f2edd182a5..943792eb43 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -244,9 +244,13 @@ __efi64_mb2_start: jmp x86_32_switch .Lefi_multiboot2_proto: - /* Zero EFI SystemTable and EFI ImageHandle addresses. */ + /* + * Zero EFI SystemTable, EFI ImageHandle and + * dom0 kernel module struct addresses. + */ xor %esi,%esi xor %edi,%edi + xor %r14d, %r14d /* Skip Multiboot2 information fixed part. */ lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx @@ -284,6 +288,15 @@ __efi64_mb2_start: cmove MB2_efi64_ih(%rcx),%rdi je .Lefi_mb2_next_tag + /* Get Dom0 kernel module struct address from Multiboot2 information. */ + cmpl $MULTIBOOT2_TAG_TYPE_MODULE,MB2_tag_type(%rcx) + jne .Lefi_mb2_end + + test %r14d, %r14d + cmovz %ecx, %r14d + jmp .Lefi_mb2_next_tag + +.Lefi_mb2_end: /* Is it the end of Multiboot2 information? */ cmpl $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx) je .Lrun_bs @@ -345,9 +358,12 @@ __efi64_mb2_start: /* Keep the stack aligned. Do not pop a single item off it. */ mov (%rsp),%rdi + mov %r14d, %edx + /* * efi_multiboot2() is called according to System V AMD64 ABI: - * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable. + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, + * %rdx - Dom0 kernel module struct address. */ call efi_multiboot2 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index f694a069c9..0d025ad9a5 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -3,6 +3,8 @@ * is intended to be included by common/efi/boot.c _only_, and * therefore can define arch specific global variables. */ +#include <xen/types.h> +#include <xen/multiboot2.h> #include <xen/vga.h> #include <asm/e820.h> #include <asm/edd.h> @@ -762,6 +764,10 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, gop = efi_get_gop(); + if ( dom0_kernel && dom0_kernel->mod_end > dom0_kernel->mod_start ) + efi_shim_lock((VOID *)(unsigned long)dom0_kernel->mod_start, + dom0_kernel->mod_end - dom0_kernel->mod_start); + if ( gop ) gop_mode = efi_find_gop_mode(gop, 0, 0, 0); diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c index 9bd6355ec3..7d459905fa 100644 --- a/xen/arch/x86/efi/stub.c +++ b/xen/arch/x86/efi/stub.c @@ -1,7 +1,9 @@ +#include <xen/types.h> #include <xen/efi.h> #include <xen/errno.h> #include <xen/init.h> #include <xen/lib.h> +#include <xen/multiboot2.h> #include <asm/asm_defns.h> #include <asm/efibind.h> #include <asm/page.h> @@ -29,7 +31,8 @@ asm ( ); void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, - EFI_SYSTEM_TABLE *SystemTable) + EFI_SYSTEM_TABLE *SystemTable, + multiboot2_tag_module_t *dom0_kernel) { static const CHAR16 __initconst err[] = L"Xen does not have EFI code build in!\r\nSystem halted!\r\n"; diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 63e289ab85..8ce6715b59 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -133,6 +133,7 @@ static void efi_console_set_mode(void); static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void); static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN cols, UINTN rows, UINTN depth); +static void efi_shim_lock(const VOID *Buffer, UINT32 Size); static void efi_tables(void); static void setup_efi_pci(void); static void efi_variables(void); @@ -830,6 +831,17 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, return gop_mode; } +static void __init efi_shim_lock(const VOID *Buffer, UINT32 Size) +{ + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; + EFI_SHIM_LOCK_PROTOCOL *shim_lock; + EFI_STATUS status; + + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, (void **)&shim_lock)) && + (status = shim_lock->Verify(Buffer, Size)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); +} + static void __init efi_tables(void) { unsigned int i; @@ -1123,13 +1135,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) { static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL; - static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; EFI_LOADED_IMAGE *loaded_image; EFI_STATUS status; unsigned int i, argc; CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; UINTN gop_mode = ~0; - EFI_SHIM_LOCK_PROTOCOL *shim_lock; EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; union string section = { NULL }, name; bool base_video = false; @@ -1296,10 +1306,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) - PrintErrMesg(L"Dom0 kernel image could not be verified", status); + efi_shim_lock(kernel.ptr, kernel.size); } if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )