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

[Gerald Elder-Vass <gerald.elder-vass@xxxxxxxxx>]

>> +          size == 1 && data == 0) )
>
>... any reason it's literal 1 here?

The size variable is also used as output from GetVariable and we should verify that the size of the returned data is as expected, it is simply one byte so probably not worth defining any macros to make it clearer

Gerald Elder-Vass

Senior Software Engineer

XenServer

Cambridge, UK

On Mon, Sep 8, 2025 at 9:49 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:

On 05.09.2025 14:10, Gerald Elder-Vass wrote:
> From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>
> Also cache it to avoid needing to repeatedly ask the firmware.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Signed-off-by: Gerald Elder-Vass <gerald.elder-vass@xxxxxxxxx>
> ---
> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> CC: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> v4:
> - Fix MISRA warning regarding SecureBoot string
> v3:
> - Fix build on ARM
> ---
>  xen/common/efi/boot.c    | 24 ++++++++++++++++++++++++
>  xen/common/efi/runtime.c |  1 +
>  xen/include/xen/efi.h    |  2 ++
>  3 files changed, 27 insertions(+)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index e12fa1a7ec04..ccbfc401f7ba 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -901,6 +901,28 @@ static void __init pre_parse(const struct file *file)
>                     " last line will be ignored.\r\n");
>  }
> 
> +static void __init init_secure_boot_mode(void)
> +{
> +    static EFI_GUID __initdata gv_uuid = EFI_GLOBAL_VARIABLE;
> +    static CHAR16 __initdata str_SecureBoot[] = L"SecureBoot";
> +    EFI_STATUS status;
> +    uint8_t data = ""> > +    UINTN size = sizeof(data);

Unlike here, ...

> +    UINT32 attr = 0;
> +
> +    status = efi_rs->GetVariable(str_SecureBoot, &gv_uuid, &attr, &size, &data);
> +
> +    if ( status == EFI_NOT_FOUND ||
> +         (status == EFI_SUCCESS &&
> +          attr == (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) &&

(Nit: Overlong line.)

> +          size == 1 && data == 0) )

... any reason it's literal 1 here?

Jan