Re: [PATCH v3 5/5] xen/x86/efi: Verify dom0 kernel with SHIM_LOCK protocol in efi_multiboot2()

[Jan Beulich <jbeulich@xxxxxxxx>]

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