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@xxxxxxx> 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@xxxxxxx>
> ---
> 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
>