diff mbox series

[v3] efi_loader: update SetVariable attribute check

Message ID 20230215095555.30150-1-masahisa.kojima@linaro.org
State New
Headers show
Series [v3] efi_loader: update SetVariable attribute check | expand

Commit Message

Masahisa Kojima Feb. 15, 2023, 9:55 a.m. UTC
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 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 | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

Comments

Heinrich Schuchardt Feb. 15, 2023, 1:14 p.m. UTC | #1
On 2/15/23 10:55, 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 v3:
> - accept no access attribute for deletion
>
> Changes in v2:
> - fix coding style
> - HR must be set with NV

The 2.10 spec say NV, BS, RT, HR.
8.2.8.2 Hardware Error Record Variables, p. 224

If we want to check HR variables, should RT be required too?

Best regards

Heinrich

>
>   lib/efi_loader/efi_variable.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 4c85cfa607..3e11373331 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -230,9 +230,30 @@ 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;
> +
> +	/* 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 +302,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 +319,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;
>   		}
Masahisa Kojima Feb. 16, 2023, 2:36 a.m. UTC | #2
Hi Heinrich,

On Wed, 15 Feb 2023 at 22:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/15/23 10:55, 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 v3:
> > - accept no access attribute for deletion
> >
> > Changes in v2:
> > - fix coding style
> > - HR must be set with NV
>
> The 2.10 spec say NV, BS, RT, HR.
> 8.2.8.2 Hardware Error Record Variables, p. 224
>
> If we want to check HR variables, should RT be required too?

Current implementation(check HR and NV only) is aligned to EDK2
reference implementation,
but there is no reason to blindly follow the EDK2.
So we will add to check RT(and implicitly BS in this case) for HR variables.

UEFI specification v2.10 P.80 and P.82 says the "HwErrRecSupport"
variable indicates
whether the platform supports Hardware Error Record Persistence or not.
We also need to check this variable existence and value.

Thanks,
Masahisa Kojima


>
> Best regards
>
> Heinrich
>
> >
> >   lib/efi_loader/efi_variable.c | 32 +++++++++++++++++++++++++-------
> >   1 file changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 4c85cfa607..3e11373331 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -230,9 +230,30 @@ 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;
> > +
> > +     /* 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 +302,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 +319,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 mbox series

Patch

diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 4c85cfa607..3e11373331 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -230,9 +230,30 @@  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;
+
+	/* 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 +302,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 +319,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;
 		}