[PATCH v4] efi_loader: update SetVariable attribute check

[Heinrich Schuchardt]

On 2/20/23 11:31, 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.
>
> UEFI specification says "HwErrRecSupport" variable supplies the level
> of support for Hardware Error Record Persistence, add check of
> this variable for EFI_VARIABLE_HARDWARE_ERROR_RECORD attribute.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> ---
> Changes in v4:
> - update HR attribute check(need NV,BS,RT)
> - check HwErrRecSupport variable for HR variables
>
> Changes in v3:
> - accept no access attribute for deletion
>
> Changes in v2:
> - fix coding style
> - HR must be set with NV
>
>   lib/efi_loader/efi_variable.c | 43 +++++++++++++++++++++++++++++------
>   1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 5804f69954..f25006badf 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -230,11 +230,43 @@ 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 && !data)
> +		return EFI_INVALID_PARAMETER;
> +
> +	/* EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated */
> +	if (attributes & EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS)
> +		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) {
> +		efi_uintn_t size;
> +		u16 hw_err_rec_support = 0;
> +
> +		/* Make sure HR is set with NV, BS and RT */
> +		if (!(attributes & EFI_VARIABLE_NON_VOLATILE) ||
> +		    !(attributes & EFI_VARIABLE_RUNTIME_ACCESS) ||
> +		    !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS))
> +			return EFI_INVALID_PARAMETER;
> +
> +		size = sizeof(hw_err_rec_support);
> +		ret = efi_get_variable_mem(L"HwErrRecSupport", &efi_global_variable_guid,
> +					   NULL, &size, &hw_err_rec_support, NULL);

The specification says:

HwErrRecSupport - Identifies the level of hardware error record
persistence support implemented by the platform. This variable is only
modified by firmware and is read-only to the OS.

Why do we have to read the variable if it is (or is not) provided by U-Boot?

Best regards

Heinrich

> +		if (ret == EFI_NOT_FOUND || !hw_err_rec_support)
> +			return EFI_UNSUPPORTED;
> +	}
> +
>   	/* check if a variable exists */
>   	var = efi_var_mem_find(vendor, variable_name, NULL);
>   	append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
> @@ -281,8 +313,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 +330,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;
>   		}