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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, May 6, 2025 at 5:56 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> 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.
>

On a more high level. Is secure boot specific to x86? I think it's
generic so it should be compiled also for ARM or any EFI enabled
architectures.

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

Frediano