[PATCH] efi_loader: update SetVariable attribute check
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Feb 14 12:15:01 CET 2023
On 2/14/23 10:19, Masahisa Kojima wrote:
> UEFI specification v2.10 says that
> EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and
> EFI_UNSUPPORTED should be returned in SetVariable variable service.
> Current implementation returns EFI_INVALID_PARAMETER,
> let's fix the return value.
>
> Together with above change, this commit also updates the SetVariable
> attribute check to be aligned with the EDK2 reference implementation.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> ---
> lib/efi_loader/efi_variable.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 4c85cfa607..1076ff7585 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -230,9 +230,28 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
> u64 time = 0;
> enum efi_auth_var_type var_type;
>
> - if (!variable_name || !*variable_name || !vendor ||
> - ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
> - !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)))
> + if (!variable_name || !*variable_name || !vendor)
> + return EFI_INVALID_PARAMETER;
> +
> + if (data_size != 0 && !data)
We tend to not use '!= 0' and ' == 0' in logical constraints. You could use
if (data_size && !data)
instead.
> + return EFI_INVALID_PARAMETER;
> +
> + /* EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated */
> + if (attributes & EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS ||
> + ((attributes & EFI_VARIABLE_MASK) == 0))
!(attributes & EFI_VARIABLE_MASK)
> + return EFI_UNSUPPORTED;
> +
> + /* Make sure if runtime bit is set, boot service bit is set also */
> + if ((attributes &
> + (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)) ==
> + EFI_VARIABLE_RUNTIME_ACCESS)
> + return EFI_INVALID_PARAMETER;
> +
> + /* only EFI_VARIABLE_NON_VOLATILE attribute is invalid */
> + if ((attributes & EFI_VARIABLE_MASK) == EFI_VARIABLE_NON_VOLATILE)
> + return EFI_INVALID_PARAMETER;
> +
> + if (attributes & EFI_VARIABLE_HARDWARE_ERROR_RECORD)
> return EFI_INVALID_PARAMETER;
Variables with flags NV, BS, RT, HR, name HwErrRec####, and GUID
EFI_HARDWARE_ERROR_VARIABLE are allowable according to the UEFI spec.
Why do we return EFI_INVALID_PARAMETER here?
Please, sort the checks above such that all EFI_INVALID_PARAMETER are
together followed by EFI_UNSUPPORTED.
Best regards
Heinrich
>
> /* check if a variable exists */
> @@ -281,8 +300,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>
> /* authenticate a variable */
> if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) {
> - if (attributes & EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS)
> - return EFI_INVALID_PARAMETER;
> if (attributes &
> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) {
> u32 env_attr;
> @@ -300,8 +317,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
> }
> } else {
> if (attributes &
> - (EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS |
> - EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)) {
> + EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) {
> EFI_PRINT("Secure boot is not configured\n");
> return EFI_INVALID_PARAMETER;
> }
More information about the U-Boot
mailing list