Re: [PATCH] efi_loader: update SetVariable attribute check

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@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;
  

  }