[PATCH v4] efi_loader: update SetVariable attribute check

[Masahisa Kojima]

Hi Heinrich,

On Mon, 20 Feb 2023 at 22:44, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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?

UEFI spec says(P.2040):

The OS determines if a platform implements support for Hardware Error
Record Persistence by reading the HwErrRecSupport globally defined
variable.

So I remove "HwErrRecSupport" check in U-Boot.

Regards,
Masahisa Kojima

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