Re: [PATCH 2/4] efi: Add a function to check if Secure Boot mode is enabled

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

CC'ing the EFI maintainers.

On 06/05/2025 5:24 pm, Kevin Lampis wrote:
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index e39fbc3529..7c528cd5dd 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -870,6 +870,27 @@ static void __init pre_parse(const struct file *file)
>                     " last line will be ignored.\r\n");
>  }
>  
> +static void __init init_secure_boot_mode(void)
> +{
> +    EFI_STATUS status;
> +    EFI_GUID gv_uuid = EFI_GLOBAL_VARIABLE;
> +    uint8_t data = 0;
> +    UINTN size = sizeof(data);
> +    UINT32 attr = 0;

Newline between variables and code please.

> +    status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &gv_uuid, &attr,
> +                                 &size, &data);
> +
> +    if ( status == EFI_NOT_FOUND ||
> +         (status == EFI_SUCCESS &&
> +          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS | 
> EFI_VARIABLE_RUNTIME_ACCESS) &&
> +          size == 1 && data == 0) )
> +        /* Platform does not support Secure Boot or it's disabled. */
> +        efi_secure_boot = false;
> +    else
> +        /* Everything else play it safe and assume enabled. */
> +        efi_secure_boot = true;
> +}

I'm not sure this logic does what you want when a weird answer comes
back from GetVariable().

Also, you can't have this be a common function, yet ...

> diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
> index 7e1fce291d..b63d21f16c 100644
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -40,6 +40,9 @@ void efi_rs_leave(struct efi_rs_state *state);
>  unsigned int __read_mostly efi_num_ct;
>  const EFI_CONFIGURATION_TABLE *__read_mostly efi_ct;
>  
> +#if defined(CONFIG_X86) && !defined(CONFIG_PV_SHIM)
> +bool __ro_after_init efi_secure_boot;
> +#endif

... this variable exist only on x86.

Also, why adjust it for PV shim?  None of this is even compiled for PV shim.

~Andrew