From: Daniel Kiper <daniel.kiper@oracle.com>

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 <daniel.kiper@oracle.com>
Signed-off-by: Bobby Eshleman <bobbyeshleman@gmail.com>
---
 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.
          */
         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) )
-- 
2.30.0

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