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
