| Message ID | 20240328151106.1451104-3-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Multiboot PE support | expand |
On 28.03.2024 16:11, Ross Lagerwall wrote: > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -3,6 +3,7 @@ > * is intended to be included by common/efi/boot.c _only_, and > * therefore can define arch specific global variables. > */ > +#include <xen/multiboot2.h> > #include <xen/vga.h> > #include <asm/e820.h> > #include <asm/edd.h> > @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt) > return o; > } > > +#define ALIGN_UP(arg, align) \ > + (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) Nit: I don't think aligning the opening parentheses is an appropriate criteria here. Imo either #define ALIGN_UP(arg, align) \ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) or #define ALIGN_UP(arg, align) \ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) or #define ALIGN_UP(arg, align) \ (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) . > +static void __init efi_verify_dom0(uint64_t mbi_in) > +{ > + uint64_t ptr; > + const multiboot2_tag_t *tag; > + EFI_SHIM_LOCK_PROTOCOL *shim_lock; > + EFI_STATUS status; > + const multiboot2_tag_module_t *kernel = NULL; > + const multiboot2_fixed_t *mbi_fix = _p(mbi_in); > + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; > + static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE; > + > + ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN); > + > + for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size; > + tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) ) > + { > + if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE ) > + { > + kernel = (const multiboot2_tag_module_t *)tag; > + break; This could do with a comment along the lines of what __start_xen() has ("Dom0 kernel is always first"). > + } > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_END ) Not need for "else" here (personally I find such irritating). > + break; > + } > + > + if ( !kernel ) > + return; > + > + if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, > + (void **)&shim_lock)) != EFI_SUCCESS ) > + { > + UINT32 attr; > + UINT8 data; > + UINTN size = sizeof(data); > + > + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid, > + &attr, &size, &data); > + if ( status == EFI_NOT_FOUND ) > + return; > + > + if ( EFI_ERROR(status) ) > + PrintErrMesg(L"Could not get SecureBoot variable", status); > + > + if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) ) > + PrintErrMesg(L"Unexpected SecureBoot attributes", attr); This wants to be blexit(), not PrintErrMesg(). > + if ( size == 1 && data == 0 ) > + return; > + > + blexit(L"Could not locate shim but Secure Boot is enabled"); > + } > + > + if ( (status = shim_lock->Verify(_p(kernel->mod_start), > + kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS ) > + PrintErrMesg(L"Dom0 kernel image could not be verified", status); > +} Overall this is a superset of what efi_start() does. What I'm missing from the description is some discussion of why what's done there is not sufficient (beyond the env var check, which iirc there once was a patch to add it). One could only then judge whether it wouldn't make sense to make this function uniformly used by both paths (with mbi_in suitably dealt with for the other case). Jan
On Mon, Apr 8, 2024 at 11:42 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 28.03.2024 16:11, Ross Lagerwall wrote: > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -3,6 +3,7 @@ > > * is intended to be included by common/efi/boot.c _only_, and > > * therefore can define arch specific global variables. > > */ > > +#include <xen/multiboot2.h> > > #include <xen/vga.h> > > #include <asm/e820.h> > > #include <asm/edd.h> > > @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt) > > return o; > > } > > > > +#define ALIGN_UP(arg, align) \ > > + (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > Nit: I don't think aligning the opening parentheses is an appropriate > criteria here. Imo either > > #define ALIGN_UP(arg, align) \ > (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > or > > #define ALIGN_UP(arg, align) \ > (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > or > > #define ALIGN_UP(arg, align) \ > (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) > > . OK, will fix. > > > +static void __init efi_verify_dom0(uint64_t mbi_in) > > +{ > > + uint64_t ptr; > > + const multiboot2_tag_t *tag; > > + EFI_SHIM_LOCK_PROTOCOL *shim_lock; > > + EFI_STATUS status; > > + const multiboot2_tag_module_t *kernel = NULL; > > + const multiboot2_fixed_t *mbi_fix = _p(mbi_in); > > + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; > > + static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE; > > + > > + ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN); > > + > > + for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size; > > + tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) ) > > + { > > + if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE ) > > + { > > + kernel = (const multiboot2_tag_module_t *)tag; > > + break; > > This could do with a comment along the lines of what __start_xen() has > ("Dom0 kernel is always first"). Will add. > > > + } > > + else if ( tag->type == MULTIBOOT2_TAG_TYPE_END ) > > Not need for "else" here (personally I find such irritating). OK, I'll remove it. > > > + break; > > + } > > + > > + if ( !kernel ) > > + return; > > + > > + if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, > > + (void **)&shim_lock)) != EFI_SUCCESS ) > > + { > > + UINT32 attr; > > + UINT8 data; > > + UINTN size = sizeof(data); > > + > > + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid, > > + &attr, &size, &data); > > + if ( status == EFI_NOT_FOUND ) > > + return; > > + > > + if ( EFI_ERROR(status) ) > > + PrintErrMesg(L"Could not get SecureBoot variable", status); > > + > > + if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) ) > > + PrintErrMesg(L"Unexpected SecureBoot attributes", attr); > > This wants to be blexit(), not PrintErrMesg(). blexit() doesn't allow printing the attributes but I can call some other function like DisplayUint() to do that before calling blexit(). > > > + if ( size == 1 && data == 0 ) > > + return; > > + > > + blexit(L"Could not locate shim but Secure Boot is enabled"); > > + } > > + > > + if ( (status = shim_lock->Verify(_p(kernel->mod_start), > > + kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS ) > > + PrintErrMesg(L"Dom0 kernel image could not be verified", status); > > +} > > Overall this is a superset of what efi_start() does. What I'm missing from > the description is some discussion of why what's done there is not > sufficient (beyond the env var check, which iirc there once was a patch to > add it). One could only then judge whether it wouldn't make sense to make > this function uniformly used by both paths (with mbi_in suitably dealt with > for the other case). > Hmm, I wasn't really looking at efi_start() verification for the purpose of this patch series. I can update the patch so that both paths use a common verification function and also describe how it differs from the simple call to shim verify that currently exists in efi_start(). Thanks, Ross
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index d8ac0f0494db..e03ae19bdafb 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -349,10 +349,12 @@ __efi64_mb2_start: /* Keep the stack aligned. Do not pop a single item off it. */ mov (%rsp),%rdi + mov %rbx, %rcx + /* * efi_multiboot2() is called according to System V AMD64 ABI: * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable, - * %rdx - MB2 cmdline + * %rdx - MB2 cmdline, %rcx - Multiboot information. */ call efi_multiboot2 diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 8ea64e31cdc2..a9569e150e08 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -3,6 +3,7 @@ * is intended to be included by common/efi/boot.c _only_, and * therefore can define arch specific global variables. */ +#include <xen/multiboot2.h> #include <xen/vga.h> #include <asm/e820.h> #include <asm/edd.h> @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt) return o; } +#define ALIGN_UP(arg, align) \ + (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1)) + +static void __init efi_verify_dom0(uint64_t mbi_in) +{ + uint64_t ptr; + const multiboot2_tag_t *tag; + EFI_SHIM_LOCK_PROTOCOL *shim_lock; + EFI_STATUS status; + const multiboot2_tag_module_t *kernel = NULL; + const multiboot2_fixed_t *mbi_fix = _p(mbi_in); + static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; + static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE; + + ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN); + + for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size; + tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) ) + { + if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE ) + { + kernel = (const multiboot2_tag_module_t *)tag; + break; + } + else if ( tag->type == MULTIBOOT2_TAG_TYPE_END ) + break; + } + + if ( !kernel ) + return; + + if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) != EFI_SUCCESS ) + { + UINT32 attr; + UINT8 data; + UINTN size = sizeof(data); + + status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid, + &attr, &size, &data); + if ( status == EFI_NOT_FOUND ) + return; + + if ( EFI_ERROR(status) ) + PrintErrMesg(L"Could not get SecureBoot variable", status); + + if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) ) + PrintErrMesg(L"Unexpected SecureBoot attributes", attr); + + if ( size == 1 && data == 0 ) + return; + + blexit(L"Could not locate shim but Secure Boot is enabled"); + } + + if ( (status = shim_lock->Verify(_p(kernel->mod_start), + kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); +} + void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable, - const char *cmdline) + const char *cmdline, uint64_t mbi_in) { EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; EFI_HANDLE gop_handle; @@ -902,6 +963,8 @@ void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle, efi_relocate_esrt(SystemTable); + efi_verify_dom0(mbi_in); + efi_exit_boot(ImageHandle, SystemTable); }
Now that the multiboot2 binary can be verified by Shim, ensure that the dom0 kernel is verified when using the multiboot2 path. If the Shim protocol is not available and the SecureBoot variable is not set to 0 (or the state cannot be determined), abort the boot. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/arch/x86/boot/head.S | 4 ++- xen/arch/x86/efi/efi-boot.h | 65 ++++++++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 2 deletions(-)