[PATCH] arm64/efi: check SetupMode when determining Secure Boot status

[PATCH] arm64/efi: check SetupMode when determining Secure Boot status

[Linn Crosetto]

According to the UEFI specification, the platform is operating in secure
boot mode if SetupMode is 0 and SecureBoot is 1, and cannot operate in
secure boot mode if SetupMode is set to 1. Check the value of SetupMode
when determining the state of Secure Boot.

Signed-off-by: Linn Crosetto <linn@hpe.com>
---
 drivers/firmware/efi/libstub/arm-stub.c | 34 +++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 3397902..7ef2e20 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -20,26 +20,36 @@
 
 static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
 {
-	static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
-	static efi_char16_t const var_name[] = {
+	static efi_char16_t const sb_var_name[] = {
 		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
+	static efi_char16_t const sm_var_name[] = {
+		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
 
+	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
 	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
-	unsigned long size = sizeof(u8);
-	efi_status_t status;
 	u8 val;
+	unsigned long size = sizeof(val);
+	efi_status_t status;
 
-	status = f_getvar((efi_char16_t *)var_name, (efi_guid_t *)&var_guid,
+	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
 			  NULL, &size, &val);
 
-	switch (status) {
-	case EFI_SUCCESS:
-		return val;
-	case EFI_NOT_FOUND:
+	if (status != EFI_SUCCESS)
 		return 0;
-	default:
-		return 1;
-	}
+
+	if (val == 0)
+		return 0;
+
+	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
+			  NULL, &size, &val);
+
+	if (status != EFI_SUCCESS)
+		return 0;
+
+	if (val == 1)
+		return 0;
+
+	return 1;
 }
 
 efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
-- 
2.1.4

Re: [PATCH] arm64/efi: check SetupMode when determining Secure Boot status

[Mark Rutland]

On Tue, Feb 23, 2016 at 05:25:09PM -0700, Linn Crosetto wrote:
> According to the UEFI specification, the platform is operating in secure
> boot mode if SetupMode is 0 and SecureBoot is 1, and cannot operate in
> secure boot mode if SetupMode is set to 1.

I see the above is from the third-last paragraph of section 3.3 Globally
Defined Variables, (in 2.5 Errata A).

For the commit message, it might be good to split the quote from the
rest of the message (e.g. by putting it in a separate indented
paragraph), to make it clear which part is from the spec.

> Check the value of SetupMode when determining the state of Secure
> Boot.

It sounds like we should be doing this. I have a couple of comments
below.

> Signed-off-by: Linn Crosetto <linn@hpe.com>
> ---
>  drivers/firmware/efi/libstub/arm-stub.c | 34 +++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 3397902..7ef2e20 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -20,26 +20,36 @@
>  
>  static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
>  {
> -	static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
> -	static efi_char16_t const var_name[] = {
> +	static efi_char16_t const sb_var_name[] = {
>  		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
> +	static efi_char16_t const sm_var_name[] = {
> +		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
>  
> +	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
>  	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
> -	unsigned long size = sizeof(u8);
> -	efi_status_t status;
>  	u8 val;
> +	unsigned long size = sizeof(val);

I think this variable could have stayed as it was, it's logically an
unrelated change. Otherwise, point out the cleanup in the commit
message.

> +	efi_status_t status;
>  
> -	status = f_getvar((efi_char16_t *)var_name, (efi_guid_t *)&var_guid,
> +	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
>  			  NULL, &size, &val);
>  
> -	switch (status) {
> -	case EFI_SUCCESS:
> -		return val;
> -	case EFI_NOT_FOUND:
> +	if (status != EFI_SUCCESS)
>  		return 0;
> -	default:
> -		return 1;
> -	}

That isn't quite the same as the existing behaviour. Previously for any
return value other than EFI_SUCCESS, we would fail-safe and assume
secure boot was enabled, whereas now we'll assume it is not.

I think we should retain the existing behaviour.

Mark.

> +
> +	if (val == 0)
> +		return 0;
> +
> +	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
> +			  NULL, &size, &val);
> +
> +	if (status != EFI_SUCCESS)
> +		return 0;
> +
> +	if (val == 1)
> +		return 0;
> +
> +	return 1;
>  }
>  
>  efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
> -- 
> 2.1.4
> 

[PATCH v2 0/2] arm64/efi: query Secure Boot status according to UEFI spec

[Linn Crosetto]

This series modifies the function that queries the status of UEFI Secure Boot
in the EFI stub to match the UEFI specification, and allow the caller to
determine if it is enabled, disabled, or in an unknown state due to an
unexpected error from GetVariable().

v2:

 - Add return values for unexpected errors
 - Split changes into two patches

Linn Crosetto (2):
  arm64/efi: report unexpected errors when determining Secure Boot status
  arm64/efi: check SetupMode when determining Secure Boot status

 drivers/firmware/efi/libstub/arm-stub.c | 49 +++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 12 deletions(-)

-- 
2.1.4

[PATCH v2 1/2] arm64/efi: report unexpected errors when determining Secure Boot status

[Linn Crosetto]

Certain code in the boot path may require the ability to determine whether
UEFI Secure Boot is definitely enabled, for example printing status to the
console. Other code may need to know when UEFI Secure Boot is definitely
disabled, for example restricting use of kernel parameters.

If an unexpected error is returned from GetVariable when querying the
status of UEFI Secure Boot, return an error to the caller. This allows the
caller to determine the definite state, and to take appropriate action if
an expected error is returned.

Signed-off-by: Linn Crosetto <linn@hpe.com>
---
New patch in v2 based on feedback from v1:

 - Maintain existing behavior to allow 'dtb=' parameter only when UEFI
   Secure Boot is disabled and not in an unknown state. (Mark Rutland)

 drivers/firmware/efi/libstub/arm-stub.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 3397902..b1bb133 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -18,7 +18,7 @@
 
 #include "efistub.h"
 
-static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
+static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
 	static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
 	static efi_char16_t const var_name[] = {
@@ -37,8 +37,12 @@ static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
 		return val;
 	case EFI_NOT_FOUND:
 		return 0;
+	case EFI_DEVICE_ERROR:
+		return -EIO;
+	case EFI_SECURITY_VIOLATION:
+		return -EACCES;
 	default:
-		return 1;
+		return -EINVAL;
 	}
 }
 
@@ -183,6 +187,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
 	unsigned long reserve_addr = 0;
 	unsigned long reserve_size = 0;
+	int secure_boot = 0;
 
 	/* Check if we were booted by the EFI firmware */
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
@@ -231,13 +236,15 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	if (status != EFI_SUCCESS)
 		pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
 
+	secure_boot = efi_get_secureboot(sys_table);
+	if (secure_boot > 0)
+		pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
+
 	/*
 	 * Unauthenticated device tree data is a security hazard, so
 	 * ignore 'dtb=' unless UEFI Secure Boot is disabled.
 	 */
-	if (efi_secureboot_enabled(sys_table)) {
-		pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
-	} else {
+	if (secure_boot == 0) {
 		status = handle_cmdline_files(sys_table, image, cmdline_ptr,
 					      "dtb=",
 					      ~0UL, &fdt_addr, &fdt_size);
-- 
2.1.4

[PATCH v2 2/2] arm64/efi: check SetupMode when determining Secure Boot status

[Linn Crosetto]

According to the UEFI specification (version 2.5 Errata A, page 87):

    The platform firmware is operating in secure boot mode if the value of
    the SetupMode variable is 0 and the SecureBoot variable is set to 1. A
    platform cannot operate in secure boot mode if the SetupMode variable
    is set to 1.

Check the value of the SetupMode variable when determining the state of
Secure Boot. Minor cleanup, change sizeof to match kernel style guidelines.

Signed-off-by: Linn Crosetto <linn@hpe.com>
---
v2:

 - Reformat quote from UEFI specification and note cleanup (Mark Rutland)
 - Restructure code on top of changes in patch 1/2

 drivers/firmware/efi/libstub/arm-stub.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b1bb133..19e54d4 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -20,21 +20,39 @@
 
 static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
-	static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
-	static efi_char16_t const var_name[] = {
+	static efi_char16_t const sb_var_name[] = {
 		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
+	static efi_char16_t const sm_var_name[] = {
+		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
 
+	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
 	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
-	unsigned long size = sizeof(u8);
-	efi_status_t status;
 	u8 val;
+	unsigned long size = sizeof(val);
+	efi_status_t status;
 
-	status = f_getvar((efi_char16_t *)var_name, (efi_guid_t *)&var_guid,
+	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
 			  NULL, &size, &val);
 
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (val == 0)
+		return 0;
+
+	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
+			  NULL, &size, &val);
+
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (val == 1)
+		return 0;
+
+	return 1;
+
+out_efi_err:
 	switch (status) {
-	case EFI_SUCCESS:
-		return val;
 	case EFI_NOT_FOUND:
 		return 0;
 	case EFI_DEVICE_ERROR:
-- 
2.1.4

Re: [PATCH v2 0/2] arm64/efi: query Secure Boot status according to UEFI spec

[Matt Fleming]

On Thu, 25 Feb, at 05:18:13PM, Linn Crosetto wrote:
> This series modifies the function that queries the status of UEFI Secure Boot
> in the EFI stub to match the UEFI specification, and allow the caller to
> determine if it is enabled, disabled, or in an unknown state due to an
> unexpected error from GetVariable().
> 
> v2:
> 
>  - Add return values for unexpected errors
>  - Split changes into two patches
> 
> Linn Crosetto (2):
>   arm64/efi: report unexpected errors when determining Secure Boot status
>   arm64/efi: check SetupMode when determining Secure Boot status
> 
>  drivers/firmware/efi/libstub/arm-stub.c | 49 +++++++++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 12 deletions(-)

Both of these look fine to me.

Mark, since you gave feedback on the first version, are you happy with
v2?

Re: [PATCH v2 1/2] arm64/efi: report unexpected errors when determining Secure Boot status

[Ard Biesheuvel]

Hi Linn,

Apologies for the delay in reviewing this.

On 26 February 2016 at 01:18, Linn Crosetto <linn@hpe.com> wrote:
> Certain code in the boot path may require the ability to determine whether
> UEFI Secure Boot is definitely enabled, for example printing status to the
> console. Other code may need to know when UEFI Secure Boot is definitely
> disabled, for example restricting use of kernel parameters.
>
> If an unexpected error is returned from GetVariable when querying the
> status of UEFI Secure Boot, return an error to the caller. This allows the
> caller to determine the definite state, and to take appropriate action if
> an expected error is returned.
>
> Signed-off-by: Linn Crosetto <linn@hpe.com>
> ---
> New patch in v2 based on feedback from v1:
>
>  - Maintain existing behavior to allow 'dtb=' parameter only when UEFI
>    Secure Boot is disabled and not in an unknown state. (Mark Rutland)
>
>  drivers/firmware/efi/libstub/arm-stub.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 3397902..b1bb133 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -18,7 +18,7 @@
>
>  #include "efistub.h"
>
> -static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
> +static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>  {
>         static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
>         static efi_char16_t const var_name[] = {
> @@ -37,8 +37,12 @@ static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
>                 return val;
>         case EFI_NOT_FOUND:
>                 return 0;
> +       case EFI_DEVICE_ERROR:
> +               return -EIO;
> +       case EFI_SECURITY_VIOLATION:
> +               return -EACCES;
>         default:
> -               return 1;
> +               return -EINVAL;
>         }
>  }
>
> @@ -183,6 +187,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>         efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
>         unsigned long reserve_addr = 0;
>         unsigned long reserve_size = 0;
> +       int secure_boot = 0;
>
>         /* Check if we were booted by the EFI firmware */
>         if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> @@ -231,13 +236,15 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>         if (status != EFI_SUCCESS)
>                 pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
>
> +       secure_boot = efi_get_secureboot(sys_table);
> +       if (secure_boot > 0)
> +               pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
> +
>         /*
>          * Unauthenticated device tree data is a security hazard, so
>          * ignore 'dtb=' unless UEFI Secure Boot is disabled.
>          */
> -       if (efi_secureboot_enabled(sys_table)) {
> -               pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
> -       } else {
> +       if (secure_boot == 0) {

There is a slight difference in behavior here: if we can't determine
whether secure boot is enabled, we no longer print anything, but
silently ignore the dtb= parameter.

Perhaps it is better to print 'could not determine secure boot status,
assuming enabled' or something like that?


>                 status = handle_cmdline_files(sys_table, image, cmdline_ptr,
>                                               "dtb=",
>                                               ~0UL, &fdt_addr, &fdt_size);
> --
> 2.1.4
>

[PATCH v3 0/2] arm64/efi: query Secure Boot status according to UEFI spec

[Linn Crosetto]

This series modifies the function that queries the status of UEFI Secure Boot
in the EFI stub to match the UEFI specification, and allow the caller to
determine if it is enabled, disabled, or in an unknown state due to an
unexpected error from GetVariable().

v2:
 - Add return values for unexpected errors
 - Split changes into two patches

v3:
 - Add more verbosity with additional prints

Linn Crosetto (2):
  arm64/efi: report unexpected errors when determining Secure Boot status
  arm64/efi: check SetupMode when determining Secure Boot status

 drivers/firmware/efi/libstub/arm-stub.c | 54 ++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 11 deletions(-)

-- 
2.1.4

[PATCH v3 1/2] arm64/efi: report unexpected errors when determining Secure Boot status

[Linn Crosetto]

Certain code in the boot path may require the ability to determine whether
UEFI Secure Boot is definitely enabled, for example printing status to the
console. Other code may need to know when UEFI Secure Boot is definitely
disabled, for example restricting use of kernel parameters.

If an unexpected error is returned from GetVariable() when querying the
status of UEFI Secure Boot, return an error to the caller. This allows the
caller to determine the definite state, and to take appropriate action if
an expected error is returned.

Signed-off-by: Linn Crosetto <linn@hpe.com>
---
v2:
 - Maintain existing behavior to allow 'dtb=' parameter only when UEFI
   Secure Boot is disabled and not in an unknown state. (Mark Rutland)

v3:
 - Add prints to inform the user in the following two cases: failure to
   determine Secure Boot status, ignoring "dtb=" kernel parameter (Ard
   Biesheuvel)

 drivers/firmware/efi/libstub/arm-stub.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 3397902..1e98fb7 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -18,7 +18,7 @@
 
 #include "efistub.h"
 
-static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
+static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
 	static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
 	static efi_char16_t const var_name[] = {
@@ -37,8 +37,12 @@ static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
 		return val;
 	case EFI_NOT_FOUND:
 		return 0;
+	case EFI_DEVICE_ERROR:
+		return -EIO;
+	case EFI_SECURITY_VIOLATION:
+		return -EACCES;
 	default:
-		return 1;
+		return -EINVAL;
 	}
 }
 
@@ -183,6 +187,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
 	unsigned long reserve_addr = 0;
 	unsigned long reserve_size = 0;
+	int secure_boot = 0;
 
 	/* Check if we were booted by the EFI firmware */
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
@@ -231,12 +236,21 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	if (status != EFI_SUCCESS)
 		pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
 
+	secure_boot = efi_get_secureboot(sys_table);
+	if (secure_boot > 0)
+		pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
+
+	if (secure_boot < 0) {
+		pr_efi_err(sys_table,
+			"could not determine UEFI Secure Boot status.\n");
+	}
+
 	/*
 	 * Unauthenticated device tree data is a security hazard, so
 	 * ignore 'dtb=' unless UEFI Secure Boot is disabled.
 	 */
-	if (efi_secureboot_enabled(sys_table)) {
-		pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
+	if (secure_boot != 0 && strstr(cmdline_ptr, "dtb=")) {
+		pr_efi(sys_table, "Ignoring DTB from command line.\n");
 	} else {
 		status = handle_cmdline_files(sys_table, image, cmdline_ptr,
 					      "dtb=",
-- 
2.1.4

[PATCH v3 2/2] arm64/efi: check SetupMode when determining Secure Boot status

[Linn Crosetto]

According to the UEFI specification (version 2.5 Errata A, page 87):

    The platform firmware is operating in secure boot mode if the value of
    the SetupMode variable is 0 and the SecureBoot variable is set to 1. A
    platform cannot operate in secure boot mode if the SetupMode variable
    is set to 1.

Check the value of the SetupMode variable when determining the state of
Secure Boot. Minor cleanup, change sizeof to match kernel style guidelines.

Signed-off-by: Linn Crosetto <linn@hpe.com>
---
v2:
 - Reformat quote from UEFI specification and note cleanup (Mark Rutland)
 - Restructure code on top of changes in patch 1/2

 drivers/firmware/efi/libstub/arm-stub.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 1e98fb7..c049d41 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -20,21 +20,39 @@
 
 static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
-	static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
-	static efi_char16_t const var_name[] = {
+	static efi_char16_t const sb_var_name[] = {
 		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
+	static efi_char16_t const sm_var_name[] = {
+		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
 
+	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
 	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
-	unsigned long size = sizeof(u8);
-	efi_status_t status;
 	u8 val;
+	unsigned long size = sizeof(val);
+	efi_status_t status;
 
-	status = f_getvar((efi_char16_t *)var_name, (efi_guid_t *)&var_guid,
+	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
 			  NULL, &size, &val);
 
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (val == 0)
+		return 0;
+
+	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
+			  NULL, &size, &val);
+
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (val == 1)
+		return 0;
+
+	return 1;
+
+out_efi_err:
 	switch (status) {
-	case EFI_SUCCESS:
-		return val;
 	case EFI_NOT_FOUND:
 		return 0;
 	case EFI_DEVICE_ERROR:
-- 
2.1.4

Re: [PATCH v3 1/2] arm64/efi: report unexpected errors when determining Secure Boot status

[Ard Biesheuvel]

On 3 March 2016 at 22:45, Linn Crosetto <linn@hpe.com> wrote:
> Certain code in the boot path may require the ability to determine whether
> UEFI Secure Boot is definitely enabled, for example printing status to the
> console. Other code may need to know when UEFI Secure Boot is definitely
> disabled, for example restricting use of kernel parameters.
>
> If an unexpected error is returned from GetVariable() when querying the
> status of UEFI Secure Boot, return an error to the caller. This allows the
> caller to determine the definite state, and to take appropriate action if
> an expected error is returned.
>
> Signed-off-by: Linn Crosetto <linn@hpe.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
> v2:
>  - Maintain existing behavior to allow 'dtb=' parameter only when UEFI
>    Secure Boot is disabled and not in an unknown state. (Mark Rutland)
>
> v3:
>  - Add prints to inform the user in the following two cases: failure to
>    determine Secure Boot status, ignoring "dtb=" kernel parameter (Ard
>    Biesheuvel)
>
>  drivers/firmware/efi/libstub/arm-stub.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 3397902..1e98fb7 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -18,7 +18,7 @@
>
>  #include "efistub.h"
>
> -static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
> +static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>  {
>         static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
>         static efi_char16_t const var_name[] = {
> @@ -37,8 +37,12 @@ static int efi_secureboot_enabled(efi_system_table_t *sys_table_arg)
>                 return val;
>         case EFI_NOT_FOUND:
>                 return 0;
> +       case EFI_DEVICE_ERROR:
> +               return -EIO;
> +       case EFI_SECURITY_VIOLATION:
> +               return -EACCES;
>         default:
> -               return 1;
> +               return -EINVAL;
>         }
>  }
>
> @@ -183,6 +187,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>         efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
>         unsigned long reserve_addr = 0;
>         unsigned long reserve_size = 0;
> +       int secure_boot = 0;
>
>         /* Check if we were booted by the EFI firmware */
>         if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> @@ -231,12 +236,21 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>         if (status != EFI_SUCCESS)
>                 pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
>
> +       secure_boot = efi_get_secureboot(sys_table);
> +       if (secure_boot > 0)
> +               pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
> +
> +       if (secure_boot < 0) {
> +               pr_efi_err(sys_table,
> +                       "could not determine UEFI Secure Boot status.\n");
> +       }
> +
>         /*
>          * Unauthenticated device tree data is a security hazard, so
>          * ignore 'dtb=' unless UEFI Secure Boot is disabled.
>          */
> -       if (efi_secureboot_enabled(sys_table)) {
> -               pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
> +       if (secure_boot != 0 && strstr(cmdline_ptr, "dtb=")) {
> +               pr_efi(sys_table, "Ignoring DTB from command line.\n");
>         } else {
>                 status = handle_cmdline_files(sys_table, image, cmdline_ptr,
>                                               "dtb=",
> --
> 2.1.4
>

Re: [PATCH v3 2/2] arm64/efi: check SetupMode when determining Secure Boot status

[Ard Biesheuvel]

On 3 March 2016 at 22:45, Linn Crosetto <linn@hpe.com> wrote:
> According to the UEFI specification (version 2.5 Errata A, page 87):
>
>     The platform firmware is operating in secure boot mode if the value of
>     the SetupMode variable is 0 and the SecureBoot variable is set to 1. A
>     platform cannot operate in secure boot mode if the SetupMode variable
>     is set to 1.
>
> Check the value of the SetupMode variable when determining the state of
> Secure Boot. Minor cleanup, change sizeof to match kernel style guidelines.
>
> Signed-off-by: Linn Crosetto <linn@hpe.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
> v2:
>  - Reformat quote from UEFI specification and note cleanup (Mark Rutland)
>  - Restructure code on top of changes in patch 1/2
>
>  drivers/firmware/efi/libstub/arm-stub.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 1e98fb7..c049d41 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -20,21 +20,39 @@
>
>  static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>  {
> -       static efi_guid_t const var_guid = EFI_GLOBAL_VARIABLE_GUID;
> -       static efi_char16_t const var_name[] = {
> +       static efi_char16_t const sb_var_name[] = {
>                 'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
> +       static efi_char16_t const sm_var_name[] = {
> +               'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
>
> +       efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
>         efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
> -       unsigned long size = sizeof(u8);
> -       efi_status_t status;
>         u8 val;
> +       unsigned long size = sizeof(val);
> +       efi_status_t status;
>
> -       status = f_getvar((efi_char16_t *)var_name, (efi_guid_t *)&var_guid,
> +       status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
>                           NULL, &size, &val);
>
> +       if (status != EFI_SUCCESS)
> +               goto out_efi_err;
> +
> +       if (val == 0)
> +               return 0;
> +
> +       status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
> +                         NULL, &size, &val);
> +
> +       if (status != EFI_SUCCESS)
> +               goto out_efi_err;
> +
> +       if (val == 1)
> +               return 0;
> +
> +       return 1;
> +
> +out_efi_err:
>         switch (status) {
> -       case EFI_SUCCESS:
> -               return val;
>         case EFI_NOT_FOUND:
>                 return 0;
>         case EFI_DEVICE_ERROR:
> --
> 2.1.4
>

Re: [PATCH v3 0/2] arm64/efi: query Secure Boot status according to UEFI spec

[Mark Rutland]

On Thu, Mar 03, 2016 at 02:45:48PM -0700, Linn Crosetto wrote:
> This series modifies the function that queries the status of UEFI Secure Boot
> in the EFI stub to match the UEFI specification, and allow the caller to
> determine if it is enabled, disabled, or in an unknown state due to an
> unexpected error from GetVariable().
> 
> v2:
>  - Add return values for unexpected errors
>  - Split changes into two patches
> 
> v3:
>  - Add more verbosity with additional prints
> 
> Linn Crosetto (2):
>   arm64/efi: report unexpected errors when determining Secure Boot status
>   arm64/efi: check SetupMode when determining Secure Boot status
> 
>  drivers/firmware/efi/libstub/arm-stub.c | 54 ++++++++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 11 deletions(-)

For the series:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark

Re: [PATCH v3 0/2] arm64/efi: query Secure Boot status according to UEFI spec

[Matt Fleming]

On Thu, 03 Mar, at 02:45:48PM, Linn Crosetto wrote:
> This series modifies the function that queries the status of UEFI Secure Boot
> in the EFI stub to match the UEFI specification, and allow the caller to
> determine if it is enabled, disabled, or in an unknown state due to an
> unexpected error from GetVariable().
> 
> v2:
>  - Add return values for unexpected errors
>  - Split changes into two patches
> 
> v3:
>  - Add more verbosity with additional prints
> 
> Linn Crosetto (2):
>   arm64/efi: report unexpected errors when determining Secure Boot status
>   arm64/efi: check SetupMode when determining Secure Boot status
> 
>  drivers/firmware/efi/libstub/arm-stub.c | 54 ++++++++++++++++++++++++++-------
>  1 file changed, 43 insertions(+), 11 deletions(-)
> 

Thanks Linn, I've picked these two patches up with Ard's and Mark's
tags.