grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to


From: Daniel Kiper
Subject: Re: [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to the Linux kernel
Date: Thu, 18 Oct 2018 11:53:52 +0200
User-agent: Mutt/1.3.28i

On Wed, Oct 17, 2018 at 06:01:46PM +0000, Ignat Korchagin wrote:
> Linux kernel from 4.11 has secure_boot member as part of linux_kernel_params.
> Currently, GRUB does not populate it, so the kernel reports
> "Secure boot could not be determined" on boot. We can populate it in EFI mode,
> so the kernel "knows" the status.
>
> Signed-off-by: Ignat Korchagin <address@hidden>
> ---
>  grub-core/loader/i386/linux.c | 54 
> ++++++++++++++++++++++++++++++++++++++++++-
>  include/grub/i386/linux.h     | 14 +++++++++--
>  2 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index 4eab55a2d..7016974a6 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -396,6 +396,57 @@ grub_linux_boot_mmap_fill (grub_uint64_t addr, 
> grub_uint64_t size,
>    return 0;
>  }
>
> +#ifdef GRUB_MACHINE_EFI
> +
> +/* from 
> https://github.com/rhboot/shim/blob/b953468e91eac48d2e3817f18cd604e20f39c56b/lib/guid.c#L39
>  */

Just mention that this comes from UEFI shim project. This should suffice.

> +#define GRUB_EFI_SHIM_LOCK_GUID \
> +  { 0x605dab50, 0xe046, 0xe046, { 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 
> 0x23 }}

This is not Linux specific. Could you add this to
include/grub/efi/api.h (Hmmm... I do not see better place)?
And I am working on patchset which will this too.
So, I will avoid some code shuffling.

> +
> +/* mostly taken from 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/firmware/efi/libstub/secureboot.c?h=linux-4.18.y&id=a7012bdbdf406bbaa4e3de0cc3d8eb0faaacbf93#n37

Stable version number will suffice here.

> +   except for the case, when "SecureBoot" variable is not found, because
> +   grub_efi_get_variable does not report EFI_STATUS to the caller */

So, I would like to ask you to change grub_efi_get_variable()
accordingly (same for grub_efi_get_variable_with_attributes()). This will
not be a big effort. It is called in a few places only. And I think that
it should work like get_efi_var() in Linux kernel. So, EFI status should
be returned and it should get pointer to variable store, e.g. &secure_boot.

And please add comment in the following way:
 /*
  *
  ...
  *
  */

> +static grub_uint8_t
> +grub_efi_secureboot_mode (void)
> +{
> +  grub_efi_guid_t efi_var_guid = GRUB_EFI_GLOBAL_VARIABLE_GUID;
> +  grub_size_t efi_var_size = 0;
> +  grub_efi_uint32_t attr = 0;
> +  grub_uint8_t *secure_boot;
> +  grub_uint8_t *setup_mode;
> +  grub_uint8_t *moksb_state;
> +  grub_uint8_t secureboot_mode = LINUX_EFI_SECUREBOOT_MODE_UNKNOWN;
> +
> +  secure_boot = grub_efi_get_variable ("SecureBoot", &efi_var_guid, 
> &efi_var_size);
> +  setup_mode = grub_efi_get_variable ("SetupMode", &efi_var_guid, 
> &efi_var_size);
> +  efi_var_guid = GRUB_EFI_SHIM_LOCK_GUID;
> +  moksb_state = grub_efi_get_variable_with_attributes ("MokSBState", 
> &efi_var_guid, &efi_var_size, &attr);

Please move these two lines...

> +  if (!secure_boot || !setup_mode)
> +    goto fail;
> +
> +  if ((*secure_boot == 0) || (*setup_mode == 1))
> +    secureboot_mode = LINUX_EFI_SECUREBOOT_MODE_DISABLED;
> +  else
> +    secureboot_mode = LINUX_EFI_SECUREBOOT_MODE_ENABLED;

...here.

> +  if (moksb_state)
> +    {
> +      if (!(attr & GRUB_EFI_VARIABLE_RUNTIME_ACCESS) && *moksb_state == 1)
> +        secureboot_mode = LINUX_EFI_SECUREBOOT_MODE_DISABLED;
> +    }

Curly brackets are not needed here.

> +fail:
> +  if (moksb_state)
> +    grub_free (moksb_state);
> +  if (setup_mode)
> +    grub_free (setup_mode);
> +  if (secure_boot)
> +    grub_free (secure_boot);
> +
> +  return secureboot_mode;
> +}
> +#endif
> +
>  static grub_err_t
>  grub_linux_boot (void)
>  {
> @@ -574,6 +625,7 @@ grub_linux_boot (void)
>      grub_efi_uintn_t efi_desc_size;
>      grub_size_t efi_mmap_target;
>      grub_efi_uint32_t efi_desc_version;
> +    ctx.params->secure_boot = grub_efi_secureboot_mode ();
>      err = grub_efi_finish_boot_services (&efi_mmap_size, efi_mmap_buf, NULL,
>                                        &efi_desc_size, &efi_desc_version);
>      if (err)
> @@ -760,7 +812,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ 
> ((unused)),
>
>    linux_params.code32_start = prot_mode_target + lh.code32_start - 
> GRUB_LINUX_BZIMAGE_ADDR;
>    linux_params.kernel_alignment = (1 << align);
> -  linux_params.ps_mouse = linux_params.padding10 =  0;
> +  linux_params.ps_mouse = linux_params.padding11 =  0;
>
>    len = sizeof (linux_params) - sizeof (lh);
>    if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) != 
> len)
> diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
> index 60c7c3b5e..4d20abb2e 100644
> --- a/include/grub/i386/linux.h
> +++ b/include/grub/i386/linux.h
> @@ -87,6 +87,12 @@ enum
>      GRUB_VIDEO_LINUX_TYPE_SIMPLE = 0x70    /* Linear framebuffer without any 
> additional functions.  */
>    };
>
> +/* Possible values for Linux secure_boot kernel parameter */
> +#define LINUX_EFI_SECUREBOOT_MODE_UNSET    0
> +#define LINUX_EFI_SECUREBOOT_MODE_UNKNOWN  1
> +#define LINUX_EFI_SECUREBOOT_MODE_DISABLED 2
> +#define LINUX_EFI_SECUREBOOT_MODE_ENABLED  3
> +
>  /* For the Linux/i386 boot protocol version 2.10.  */
>  struct linux_i386_kernel_header
>  {
> @@ -270,7 +276,11 @@ struct linux_kernel_params
>
>    grub_uint8_t mmap_size;            /* 1e8 */
>
> -  grub_uint8_t padding9[0x1f1 - 0x1e9];
> +  grub_uint8_t padding9[0x1ec - 0x1e9];
> +
> +  grub_uint8_t secure_boot;             /* 1ec */
> +
> +  grub_uint8_t padding10[0x1f1 - 0x1ed];
>
>    grub_uint8_t setup_sects;          /* The size of the setup in sectors */
>    grub_uint16_t root_flags;          /* If the root is mounted readonly */
> @@ -280,7 +290,7 @@ struct linux_kernel_params
>    grub_uint16_t vid_mode;            /* Video mode control */
>    grub_uint16_t root_dev;            /* Default root device number */
>
> -  grub_uint8_t padding10;            /* 1fe */
> +  grub_uint8_t padding11;            /* 1fe */
>    grub_uint8_t ps_mouse;             /* 1ff */

linux/arch/x86/include/uapi/asm/bootparam.h currently says that
old padding10 + ps_mouse is boot_flag. Please merge them and
use boot_flag as struct member name.

And I have a feeling that you should post a patch for Linux kernel
which adds a comment around efi_get_secureboot() and
xen_efi_get_secureboot(void) definitions saying that if these functions
logic change then relevant bootloaders, e.g. GRUB2, should be properly
updated. You can add Suggested-by: Daniel Kiper <address@hidden>
and CC me. If maintainers get it great. If no I will not insist on it.
Though I think that it is worth trying.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]