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 <ig...@cloudflare.com>
> ---
> 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 <daniel.ki...@oracle.com>
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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
