On Tue, 14 Feb 2023 at 20:15, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

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.

OK.

...

•         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)

OK.

...

•         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?

Sorry, I misunderstood EDK2 implementation. I will update to check HR must be set with NV.

454a9442fb(efi_loader: update attribute check for QueryVariableInfo()) also does wrong check for HR, I will fix it.

Please, sort the checks above such that all EFI_INVALID_PARAMETER are together followed by EFI_UNSUPPORTED.

SCT SIE(Security Interface Extension) expects that deprecated EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS attribute check is first priority. So let me prioritise EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS check. In the next version, the error check order is: - basic parameter check such as variable_name is NULL - deprecated and unsupported attribute check - invalid attribute check

Thanks, Masahisa Kojima

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