Re: [PATCH v3 5/5] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2()
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
> +#include
> #include
> #include
> #include
> @@ -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
Please don't, even less so without honoring the alphabetical sorting.
> #include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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(_lock_guid, NULL, (void
> **)_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
[PATCH v3 5/5] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2()
From: Daniel Kiper
This splits out efi_shim_lock() into common code and uses it to verify
the dom0 kernel in efi_multiboot2().
Signed-off-by: Daniel Kiper
Signed-off-by: Bobby Eshleman
---
xen/arch/x86/boot/head.S| 20 ++--
xen/arch/x86/efi/efi-boot.h | 6 ++
xen/arch/x86/efi/stub.c | 5 -
xen/common/efi/boot.c | 19 +--
4 files changed, 41 insertions(+), 9 deletions(-)
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.
*/
callefi_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
+#include
#include
#include
#include
@@ -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
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -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(_lock_guid, NULL, (void
**)_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 =
