Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled.

[Jan Beulich <jbeulich@xxxxxxxx>]

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