diff mbox series

[v3,4/4] efi: Do not use command line if secure boot is enabled.

Message ID 20200907190027.669086-5-hudson@trmm.net
State Superseded
Headers show
Series efi: Unified Xen hypervisor/kernel/initrd images | expand

Commit Message

Trammell Hudson Sept. 7, 2020, 7 p.m. UTC
From: Trammell hudson <hudson@trmm.net>

If secure boot is enabled, the Xen command line arguments are ignored.
If a unified Xen image is used, then the bundled configuration, dom0
kernel, and initrd are prefered over the ones listed in the config file.

Unlike the shim based verification, the PE signature on a unified image
covers the all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that properly configured platforms
will measure the entire runtime into the TPM for unsealing secrets or
remote attestation.

Signed-off-by: Trammell Hudson <hudson@trmm.net>
---
 xen/common/efi/boot.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

Comments

Roger Pau Monné Sept. 14, 2020, 10:24 a.m. UTC | #1
On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote:
> From: Trammell hudson <hudson@trmm.net>
> 
> If secure boot is enabled, the Xen command line arguments are ignored.
> If a unified Xen image is used, then the bundled configuration, dom0
> kernel, and initrd are prefered over the ones listed in the config file.
> 
> Unlike the shim based verification, the PE signature on a unified image
> covers the all of the Xen+config+kernel+initrd modules linked into the
> unified image. This also ensures that properly configured platforms
> will measure the entire runtime into the TPM for unsealing secrets or
> remote attestation.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>
> ---
>  xen/common/efi/boot.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 452b5f4362..5aaebd5f20 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -947,6 +947,26 @@ static void __init setup_efi_pci(void)
>      efi_bs->FreePool(handles);
>  }
>  
> +/*
> + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c
> + * Secure Boot is enabled iff 'SecureBoot' is set and the system is
> + * not in Setup Mode.
> + */
> +static bool __init efi_secure_boot(void)
> +{
> +    static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
> +    uint8_t secboot, setupmode;
> +    UINTN secboot_size = sizeof(secboot);
> +    UINTN setupmode_size = sizeof(setupmode);
> +
> +    if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &secboot_size, &secboot) != EFI_SUCCESS )

I'm slightly worried about the dropping of the const here, and the
fact that the variable is placed in initconst section. Isn't it
dangerous that the EFI services will try to write to it?

Line length also.

> +        return false;
> +    if ( efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, NULL, &setupmode_size, &setupmode) != EFI_SUCCESS )
> +        return false;
> +
> +    return secboot == 1 && setupmode == 0;

I would print a message if secboot is > 1, since those should be
reserved.

Roger.
Trammell Hudson Sept. 14, 2020, 11:36 a.m. UTC | #2
On Monday, September 14, 2020 6:24 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote:
> [...]
> > -   static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
> > -   uint8_t secboot, setupmode;
> > -   UINTN secboot_size = sizeof(secboot);
> > -   UINTN setupmode_size = sizeof(setupmode);
> > -
> > -   if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &secboot_size, &secboot) != EFI_SUCCESS )
>
> I'm slightly worried about the dropping of the const here, and the
> fact that the variable is placed in initconst section. Isn't it
> dangerous that the EFI services will try to write to it?

The EFI services do not try to write to it; the API doesn't
even bother with const-correctness.  The prototype has IN
and OUT, but they are not used for constness:

typedef EFI_STATUS(EFIAPI * EFI_GET_VARIABLE) (
IN CHAR16 *VariableName,
IN EFI_GUID *VendorGuid,
OUT UINT32 *Attributes,
OPTIONAL IN OUT UINTN *DataSize,
OUT VOID *Data OPTIONAL)

(So the VariableName string is also silently being turned
into a non-const pointer as well, which is just ugh)

> [...]
> > -   return secboot == 1 && setupmode == 0;
>
> I would print a message if secboot is > 1, since those should be
> reserved.

Ok.  Addressed in v4, coming soon.

--
Trammell
Jan Beulich Sept. 14, 2020, 12:16 p.m. UTC | #3
On 14.09.2020 13:36, Trammell Hudson wrote:
> On Monday, September 14, 2020 6:24 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
>> On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote:
>> [...]
>>> -   static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
>>> -   uint8_t secboot, setupmode;
>>> -   UINTN secboot_size = sizeof(secboot);
>>> -   UINTN setupmode_size = sizeof(setupmode);
>>> -
>>> -   if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &secboot_size, &secboot) != EFI_SUCCESS )
>>
>> I'm slightly worried about the dropping of the const here, and the
>> fact that the variable is placed in initconst section. Isn't it
>> dangerous that the EFI services will try to write to it?
> 
> The EFI services do not try to write to it; the API doesn't
> even bother with const-correctness.  The prototype has IN
> and OUT, but they are not used for constness:
> 
> typedef EFI_STATUS(EFIAPI * EFI_GET_VARIABLE) (
> IN CHAR16 *VariableName,
> IN EFI_GUID *VendorGuid,
> OUT UINT32 *Attributes,
> OPTIONAL IN OUT UINTN *DataSize,
> OUT VOID *Data OPTIONAL)

And I think this underlying aspect if the reason for a lot of apparently
missing const in our EFI interfacing code.

Jan
diff mbox series

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 452b5f4362..5aaebd5f20 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -947,6 +947,26 @@  static void __init setup_efi_pci(void)
     efi_bs->FreePool(handles);
 }
 
+/*
+ * Logic should remain sync'ed with linux/arch/x86/xen/efi.c
+ * Secure Boot is enabled iff 'SecureBoot' is set and the system is
+ * not in Setup Mode.
+ */
+static bool __init efi_secure_boot(void)
+{
+    static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
+    uint8_t secboot, setupmode;
+    UINTN secboot_size = sizeof(secboot);
+    UINTN setupmode_size = sizeof(setupmode);
+
+    if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, &secboot_size, &secboot) != EFI_SUCCESS )
+        return false;
+    if ( efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, NULL, &setupmode_size, &setupmode) != EFI_SUCCESS )
+        return false;
+
+    return secboot == 1 && setupmode == 0;
+}
+
 static void __init efi_variables(void)
 {
     EFI_STATUS status;
@@ -1123,8 +1143,8 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     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;
+    unsigned int i, argc = 0;
+    CHAR16 **argv = NULL, *file_name, *cfg_file_name = NULL, *options = NULL;
     UINTN gop_mode = ~0;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
@@ -1132,6 +1152,7 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     bool base_video = false;
     char *option_str;
     bool use_cfg_file;
+    bool secure = false;
 
     __set_bit(EFI_BOOT, &efi_flags);
     __set_bit(EFI_LOADER, &efi_flags);
@@ -1150,8 +1171,10 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         PrintErrMesg(L"No Loaded Image Protocol", status);
 
     efi_arch_load_addr_check(loaded_image);
+    secure = efi_secure_boot();
 
-    if ( use_cfg_file )
+    /* If UEFI Secure Boot is enabled, do not parse the command line */
+    if ( use_cfg_file && !secure )
     {
         UINTN offset = 0;
 
@@ -1209,6 +1232,8 @@  efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION)
              XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
+    if ( secure )
+        PrintStr(L"UEFI Secure Boot enabled\r\n");
 
     efi_arch_relocate_image(0);