diff mbox

[v2] efi: libstub: treat missing SecureBoot variable as S/B disabled

Message ID 1487268503-21570-1-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel Feb. 16, 2017, 6:08 p.m. UTC
The newly refactored code that infers the firmware's Secure Boot state
prints the following error when the variables 'SecureBoot' is missing.

  EFI stub: ERROR: Could not determine UEFI Secure Boot status.

However, this variable is only guaranteed to be defined on a system that
is Secure Boot capable to begin with, and so it is not an error if it is
missing. So report Secure Boot as being disabled in this case, without
printing any error messages.

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

---
v2: treat SecureBoot present but SetupMode missing as 'unknown' not 'disabled'

 drivers/firmware/efi/libstub/secureboot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Lukas Wunner Feb. 17, 2017, 5:13 a.m. UTC | #1
On Thu, Feb 16, 2017 at 06:08:23PM +0000, Ard Biesheuvel wrote:
> The newly refactored code that infers the firmware's Secure Boot state

> prints the following error when the variables 'SecureBoot' is missing.

> 

>   EFI stub: ERROR: Could not determine UEFI Secure Boot status.

> 

> However, this variable is only guaranteed to be defined on a system that

> is Secure Boot capable to begin with, and so it is not an error if it is

> missing. So report Secure Boot as being disabled in this case, without

> printing any error messages.


In fact I pointed out this change of behaviour on ARM during the
review process:

https://lkml.org/lkml/2016/12/8/702

But David thought otherwise. :-(

Best regards,

Lukas

> 

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

> ---

> v2: treat SecureBoot present but SetupMode missing as 'unknown' not 'disabled'

> 

>  drivers/firmware/efi/libstub/secureboot.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c

> index 766ac06dac84..8fa8af4b3ca8 100644

> --- a/drivers/firmware/efi/libstub/secureboot.c

> +++ b/drivers/firmware/efi/libstub/secureboot.c

> @@ -47,6 +47,8 @@ enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)

>  	size = sizeof(secboot);

>  	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,

>  			     NULL, &size, &secboot);

> +	if (status == EFI_NOT_FOUND)

> +		return efi_secureboot_mode_disabled;

>  	if (status != EFI_SUCCESS)

>  		goto out_efi_err;

>  

> @@ -80,7 +82,5 @@ enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)

>  

>  out_efi_err:

>  	pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");

> -	if (status == EFI_NOT_FOUND)

> -		return efi_secureboot_mode_disabled;

>  	return efi_secureboot_mode_unknown;

>  }

> -- 

> 2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Feb. 17, 2017, 7:08 a.m. UTC | #2
On 17 February 2017 at 05:13, Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, Feb 16, 2017 at 06:08:23PM +0000, Ard Biesheuvel wrote:

>> The newly refactored code that infers the firmware's Secure Boot state

>> prints the following error when the variables 'SecureBoot' is missing.

>>

>>   EFI stub: ERROR: Could not determine UEFI Secure Boot status.

>>

>> However, this variable is only guaranteed to be defined on a system that

>> is Secure Boot capable to begin with, and so it is not an error if it is

>> missing. So report Secure Boot as being disabled in this case, without

>> printing any error messages.

>

> In fact I pointed out this change of behaviour on ARM during the

> review process:

>

> https://lkml.org/lkml/2016/12/8/702

>

> But David thought otherwise. :-(

>


I do remember that discussion. But I think David catered for that by
returning enabled/disabled/unknown, deferring the decision how to deal
with 'unknown' to the caller.

But I did not appreciate at the time that this was a change in
behavior nonetheless, and printing errors that are not errors only
confuses people.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Howells Feb. 17, 2017, 8:56 a.m. UTC | #3
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> But I did not appreciate at the time that this was a change in

> behavior nonetheless, and printing errors that are not errors only

> confuses people.


Perhaps just take the error message out and leave it to the main kernel to
deal with?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel Feb. 20, 2017, 8:53 a.m. UTC | #4
> On 17 Feb 2017, at 08:56, David Howells <dhowells@redhat.com> wrote:

> 

> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> 

>> But I did not appreciate at the time that this was a change in

>> behavior nonetheless, and printing errors that are not errors only

>> confuses people.

> 

> Perhaps just take the error message out and leave it to the main kernel to

> deal with?

> 


No, a non-existent SecureBoot variable is simply not an error condition
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 766ac06dac84..8fa8af4b3ca8 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -47,6 +47,8 @@  enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	size = sizeof(secboot);
 	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
 			     NULL, &size, &secboot);
+	if (status == EFI_NOT_FOUND)
+		return efi_secureboot_mode_disabled;
 	if (status != EFI_SUCCESS)
 		goto out_efi_err;
 
@@ -80,7 +82,5 @@  enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 
 out_efi_err:
 	pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
-	if (status == EFI_NOT_FOUND)
-		return efi_secureboot_mode_disabled;
 	return efi_secureboot_mode_unknown;
 }