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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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().

What specific case do you think is handled incorrectly here?

Ross