[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |