| Message ID | 20230215022959.8370-1-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] efi_loader: update SetVariable attribute check | expand |
On 2/15/23 03:29, 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> > --- > Changes in v2: > - fix coding style > - HR must be set with NV > > lib/efi_loader/efi_variable.c | 33 ++++++++++++++++++++++++++------- > 1 file changed, 26 insertions(+), 7 deletions(-) > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 4c85cfa607..b12e79d658 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -230,9 +230,31 @@ 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 || > + (!(attributes & EFI_VARIABLE_MASK))) UEFI Spec 2.10, p. 215: "Setting a data variable with no access attributes causes it to be deleted." We don't want to make deletion unsupported. Best regards Heinrich > + 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; > + > + /* Make sure HR is set with NV */ > + if ((attributes & > + (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_HARDWARE_ERROR_RECORD)) == > + EFI_VARIABLE_HARDWARE_ERROR_RECORD) > return EFI_INVALID_PARAMETER; > > /* check if a variable exists */ > @@ -281,8 +303,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 +320,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 Wed, 15 Feb 2023 at 17:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 2/15/23 03:29, 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> > > --- > > Changes in v2: > > - fix coding style > > - HR must be set with NV > > > > lib/efi_loader/efi_variable.c | 33 ++++++++++++++++++++++++++------- > > 1 file changed, 26 insertions(+), 7 deletions(-) > > > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > index 4c85cfa607..b12e79d658 100644 > > --- a/lib/efi_loader/efi_variable.c > > +++ b/lib/efi_loader/efi_variable.c > > @@ -230,9 +230,31 @@ 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 || > > + (!(attributes & EFI_VARIABLE_MASK))) > > UEFI Spec 2.10, p. 215: "Setting a data variable with no access > attributes causes it to be deleted." > > We don't want to make deletion unsupported. Sorry, you are correct. I will remove the second condition. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > + 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; > > + > > + /* Make sure HR is set with NV */ > > + if ((attributes & > > + (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_HARDWARE_ERROR_RECORD)) == > > + EFI_VARIABLE_HARDWARE_ERROR_RECORD) > > return EFI_INVALID_PARAMETER; > > > > /* check if a variable exists */ > > @@ -281,8 +303,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 +320,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 4c85cfa607..b12e79d658 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -230,9 +230,31 @@ 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 || + (!(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; + + /* Make sure HR is set with NV */ + if ((attributes & + (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_HARDWARE_ERROR_RECORD)) == + EFI_VARIABLE_HARDWARE_ERROR_RECORD) return EFI_INVALID_PARAMETER; /* check if a variable exists */ @@ -281,8 +303,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 +320,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. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- Changes in v2: - fix coding style - HR must be set with NV lib/efi_loader/efi_variable.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)