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

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

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