| Message ID | 20230214091943.30544-1-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi_loader: update SetVariable attribute check | expand |
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. > + 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) > + 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? Please, sort the checks above such that all EFI_INVALID_PARAMETER are together followed by EFI_UNSUPPORTED. 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; > }
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; > > } >
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) + return EFI_INVALID_PARAMETER; + + /* EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated */ + if (attributes & EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS || + ((attributes & EFI_VARIABLE_MASK) == 0)) + 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; /* 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; }
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(-)