| Message ID | 20230220103148.32317-1-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] efi_loader: update SetVariable attribute check | expand |
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@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; > }
Hi Heinrich, On Mon, 20 Feb 2023 at 22:44, Heinrich Schuchardt <xypron.glpk@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@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; > > } >
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); + 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; }
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@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(-)