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

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

[Ard Biesheuvel]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

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

[Lukas Wunner]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

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

[Ard Biesheuvel]

On 17 February 2017 at 05:13, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> 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.

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

[David Howells]

Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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

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

[Ard Biesheuvel]

> On 17 Feb 2017, at 08:56, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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