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 | 44 ++++++++++++++++++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4b1fbc9643..e65c1f1a09 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -949,6 +949,39 @@ 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);
+ EFI_STATUS rc;
+
+ rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid,
+ NULL, &secboot_size, &secboot);
+ if ( rc != EFI_SUCCESS )
+ return false;
+
+ rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid,
+ NULL, &setupmode_size, &setupmode);
+ if ( rc != EFI_SUCCESS )
+ return false;
+
+ if ( secboot > 1)
+ {
+ PrintStr(L"Invalid SecureBoot variable=0x");
+ DisplayUint(secboot, 2);
+ PrintStr(newline);
+ }
+
+ return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1125,8 +1158,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;
@@ -1134,6 +1167,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);
@@ -1152,8 +1186,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;
@@ -1211,6 +1247,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);
--
2.25.1
On 14.09.2020 13:50, Trammell Hudson wrote:
> 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.
The command line may also include a part handed on to the Dom0 kernel.
If the Dom0 kernel image comes from disk, I don't see why that part of
the command line shouldn't be honored. Similarly, if the config file
doesn't come from the unified image, I think Xen's command line options
should also be honored.
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -949,6 +949,39 @@ 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);
> + EFI_STATUS rc;
> +
> + rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid,
As you need the casts here just to get rid of the const again,
please don't make the variable const in the first place.
> + NULL, &secboot_size, &secboot);
> + if ( rc != EFI_SUCCESS )
> + return false;
> +
> + rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid,
> + NULL, &setupmode_size, &setupmode);
> + if ( rc != EFI_SUCCESS )
> + return false;
> +
> + if ( secboot > 1)
> + {
> + PrintStr(L"Invalid SecureBoot variable=0x");
> + DisplayUint(secboot, 2);
> + PrintStr(newline);
> + }
> +
> + return secboot == 1 && setupmode == 0;
Like for SecureBoot, values other than 0 and 1 also are reserved for
SetupMode and hence would better be logged in the same way. I'm still
unconvinced though that logging is enough.
> @@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> bool base_video = false;
> char *option_str;
> bool use_cfg_file;
> + bool secure = false;
I don't think the initializer is needed here?
> @@ -1152,8 +1186,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 )
> {
If it is intentional to also change this for the shim case, then
please justify the change in behavior in the description. As per
the comments further up I think the ignoring of the various parts
wants making depend on more than just secure boot mode anyway.
Jan
On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 14.09.2020 13:50, Trammell Hudson wrote: > > 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. > > The command line may also include a part handed on to the Dom0 kernel. > If the Dom0 kernel image comes from disk, I don't see why that part of > the command line shouldn't be honored. Similarly, if the config file > doesn't come from the unified image, I think Xen's command line options > should also be honored. Ignoring the command line and breaking the shim behaviour in a unified image should be ok; that is an explicit decision by the system owner to sign and configure the new image (and the shim is not used in a unified image anyway). If we have a way to detect a unified image early enough, then we can avoid the backwards incompatibility if it is not unified. That would require moving the config parsing to above the relocation call. I'm testing that now to see if it works on x86. -- Trammell
On 17.09.2020 16:05, Trammell Hudson wrote: > On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote: >> On 14.09.2020 13:50, Trammell Hudson wrote: >>> 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. >> >> The command line may also include a part handed on to the Dom0 kernel. >> If the Dom0 kernel image comes from disk, I don't see why that part of >> the command line shouldn't be honored. Similarly, if the config file >> doesn't come from the unified image, I think Xen's command line options >> should also be honored. > > Ignoring the command line and breaking the shim behaviour in a > unified image should be ok; that is an explicit decision by the > system owner to sign and configure the new image (and the shim > is not used in a unified image anyway). > > If we have a way to detect a unified image early enough, then > we can avoid the backwards incompatibility if it is not unified. I was assuming this was easily possible, if necessary as about the first thing we do. If it's not as easy, perhaps something wants adding to make it so? > That would require moving the config parsing to above the relocation > call. I guess I don't understand why this would be. Jan
On Thursday, September 17, 2020 11:26 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 17.09.2020 16:05, Trammell Hudson wrote: > > If we have a way to detect a unified image early enough, then > > we can avoid the backwards incompatibility if it is not unified. > > I was assuming this was easily possible, if necessary as about the > first thing we do. If it's not as easy, perhaps something wants > adding to make it so? v5 of the patch (just sent) has changed the logic so that the config section is searched first thing, and if it is found, then and only then is the command line ignored. I believe this restores the older behaviour. > > That would require moving the config parsing to above the relocation > > call. > > I guess I don't understand why this would be. The early command line parsing happens before the call to efi_arch_relocate_image(), although testing in qemu did not seem to cause any problems with calling read_section() before the reloc. -- Trammell
© 2016 - 2026 Red Hat, Inc.