All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] loader/i386/linux: report UEFI secure boot status to the Linux kernel
@ 2018-10-09 16:04 Ignat Korchagin
  2018-10-17 14:59 ` Daniel Kiper
  0 siblings, 1 reply; 7+ messages in thread
From: Ignat Korchagin @ 2018-10-09 16:04 UTC (permalink / raw)
  To: grub-devel; +Cc: Ignat Korchagin

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 <ignat@cloudflare.com>
---
 grub-core/loader/i386/linux.c | 34 +++++++++++++++++++++++++++++++++-
 include/grub/i386/linux.h     | 12 ++++++++++--
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index 4eab55a2d..7fc188603 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -396,6 +396,37 @@ grub_linux_boot_mmap_fill (grub_uint64_t addr, grub_uint64_t size,
   return 0;
 }
 
+#ifdef GRUB_MACHINE_EFI
+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_uint8_t *secure_boot;
+  grub_uint8_t *setup_mode;
+  grub_uint8_t secureboot_mode = LINUX_EFI_SECUREBOOT_MODE_UNSET;
+
+  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);
+
+  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;
+
+fail:
+  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 +605,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 +792,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..4493a3fdb 100644
--- a/include/grub/i386/linux.h
+++ b/include/grub/i386/linux.h
@@ -270,7 +270,15 @@ 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 */
+#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
+
+  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 +288,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 */
 
   grub_uint16_t jump;			/* Jump instruction */
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] loader/i386/linux: report UEFI secure boot status to the Linux kernel
  2018-10-09 16:04 [PATCH] loader/i386/linux: report UEFI secure boot status to the Linux kernel Ignat Korchagin
@ 2018-10-17 14:59 ` Daniel Kiper
  2018-10-17 18:01   ` [PATCH v2 0/2] " Ignat Korchagin
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Kiper @ 2018-10-17 14:59 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: Ignat Korchagin

On Tue, Oct 09, 2018 at 04:04:03PM +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 <ignat@cloudflare.com>
> ---
>  grub-core/loader/i386/linux.c | 34 +++++++++++++++++++++++++++++++++-
>  include/grub/i386/linux.h     | 12 ++++++++++--
>  2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index 4eab55a2d..7fc188603 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -396,6 +396,37 @@ grub_linux_boot_mmap_fill (grub_uint64_t addr, grub_uint64_t size,
>    return 0;
>  }
>
> +#ifdef GRUB_MACHINE_EFI
> +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_uint8_t *secure_boot;
> +  grub_uint8_t *setup_mode;
> +  grub_uint8_t secureboot_mode = LINUX_EFI_SECUREBOOT_MODE_UNSET;
> +
> +  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);
> +
> +  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;
> +
> +fail:
> +  if (setup_mode)
> +    grub_free (setup_mode);
> +  if (secure_boot)
> +    grub_free (secure_boot);
> +
> +  return secureboot_mode;

May I ask you to duplicate the logic from
linux/drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot()?
Additionally, please add the comment that it is taken from there.
And it is also worth mentioning the Linux kernel version or commit id.

> +}
> +#endif
> +
>  static grub_err_t
>  grub_linux_boot (void)
>  {
> @@ -574,6 +605,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 +792,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..4493a3fdb 100644
> --- a/include/grub/i386/linux.h
> +++ b/include/grub/i386/linux.h
> @@ -270,7 +270,15 @@ 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 */
> +#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

Please mov this to constants section above.

Daniel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 0/2] report UEFI secure boot status to the Linux kernel
  2018-10-17 14:59 ` Daniel Kiper
@ 2018-10-17 18:01   ` Ignat Korchagin
  2018-10-17 18:01     ` [PATCH v2 1/2] efi: add function to read EFI variables with attributes Ignat Korchagin
  2018-10-17 18:01     ` [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to the Linux kernel Ignat Korchagin
  0 siblings, 2 replies; 7+ messages in thread
From: Ignat Korchagin @ 2018-10-17 18:01 UTC (permalink / raw)
  To: grub-devel; +Cc: Ignat Korchagin

Reworked the logic to more closely resemble the one in the Linux efistub
implementation:
  * added checking for "MokSBState" variable
  * added a function to efi core to read EFI variables with their attributes,
    because "MokSBState" handling requires taking attributes into account

Ignat Korchagin (2):
  efi: add function to read EFI variables with attributes
  loader/i386/linux: report UEFI secure boot status to the Linux kernel

 grub-core/kern/efi/efi.c      | 13 +++++++++--
 grub-core/loader/i386/linux.c | 54 ++++++++++++++++++++++++++++++++++++++++++-
 include/grub/efi/efi.h        |  4 ++++
 include/grub/i386/linux.h     | 14 +++++++++--
 4 files changed, 80 insertions(+), 5 deletions(-)

-- 
2.11.0



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] efi: add function to read EFI variables with attributes
  2018-10-17 18:01   ` [PATCH v2 0/2] " Ignat Korchagin
@ 2018-10-17 18:01     ` Ignat Korchagin
  2018-10-18  8:48       ` Daniel Kiper
  2018-10-17 18:01     ` [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to the Linux kernel Ignat Korchagin
  1 sibling, 1 reply; 7+ messages in thread
From: Ignat Korchagin @ 2018-10-17 18:01 UTC (permalink / raw)
  To: grub-devel; +Cc: Ignat Korchagin

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 grub-core/kern/efi/efi.c | 13 +++++++++++--
 include/grub/efi/efi.h   |  4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 708581fcb..5f7d1478f 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -224,7 +224,16 @@ grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,
 
 void *
 grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
-		       grub_size_t *datasize_out)
+                       grub_size_t *datasize_out)
+{
+  return grub_efi_get_variable_with_attributes (var, guid, datasize_out, NULL);
+}
+
+void *
+grub_efi_get_variable_with_attributes (const char *var,
+                                       const grub_efi_guid_t *guid,
+                                       grub_size_t *datasize_out,
+                                       grub_efi_uint32_t *attributes)
 {
   grub_efi_status_t status;
   grub_efi_uintn_t datasize = 0;
@@ -260,7 +269,7 @@ grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
       return NULL;
     }
 
-  status = efi_call_5 (r->get_variable, var16, guid, NULL, &datasize, data);
+  status = efi_call_5 (r->get_variable, var16, guid, attributes, &datasize, data);
   grub_free (var16);
 
   if (status == GRUB_EFI_SUCCESS)
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 2c6648d46..727722797 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -77,6 +77,10 @@ grub_err_t EXPORT_FUNC (grub_efi_set_virtual_address_map) (grub_efi_uintn_t memo
 void *EXPORT_FUNC (grub_efi_get_variable) (const char *variable,
 					   const grub_efi_guid_t *guid,
 					   grub_size_t *datasize_out);
+void *EXPORT_FUNC (grub_efi_get_variable_with_attributes) (const char *variable,
+                                                           const grub_efi_guid_t *guid,
+                                                           grub_size_t *datasize_out,
+                                                           grub_efi_uint32_t *attributes);
 grub_err_t
 EXPORT_FUNC (grub_efi_set_variable) (const char *var,
 				     const grub_efi_guid_t *guid,
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to the Linux kernel
  2018-10-17 18:01   ` [PATCH v2 0/2] " Ignat Korchagin
  2018-10-17 18:01     ` [PATCH v2 1/2] efi: add function to read EFI variables with attributes Ignat Korchagin
@ 2018-10-17 18:01     ` Ignat Korchagin
  2018-10-18  9:53       ` Daniel Kiper
  1 sibling, 1 reply; 7+ messages in thread
From: Ignat Korchagin @ 2018-10-17 18:01 UTC (permalink / raw)
  To: grub-devel; +Cc: Ignat Korchagin

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 <ignat@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 */
+#define GRUB_EFI_SHIM_LOCK_GUID \
+  { 0x605dab50, 0xe046, 0xe046, { 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23 }}
+
+/* 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
+   except for the case, when "SecureBoot" variable is not found, because
+   grub_efi_get_variable does not report EFI_STATUS to the caller */
+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);
+
+  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;
+
+  if (moksb_state)
+    {
+      if (!(attr & GRUB_EFI_VARIABLE_RUNTIME_ACCESS) && *moksb_state == 1)
+        secureboot_mode = LINUX_EFI_SECUREBOOT_MODE_DISABLED;
+    }
+
+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 */
 
   grub_uint16_t jump;			/* Jump instruction */
-- 
2.11.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] efi: add function to read EFI variables with attributes
  2018-10-17 18:01     ` [PATCH v2 1/2] efi: add function to read EFI variables with attributes Ignat Korchagin
@ 2018-10-18  8:48       ` Daniel Kiper
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Kiper @ 2018-10-18  8:48 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: Ignat Korchagin

On Wed, Oct 17, 2018 at 06:01:45PM +0000, Ignat Korchagin wrote:

May I ask you to add a few words here why this change is needed.

> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> ---
>  grub-core/kern/efi/efi.c | 13 +++++++++++--
>  include/grub/efi/efi.h   |  4 ++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 708581fcb..5f7d1478f 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -224,7 +224,16 @@ grub_efi_set_variable(const char *var, const grub_efi_guid_t *guid,
>
>  void *
>  grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
> -		       grub_size_t *datasize_out)
> +                       grub_size_t *datasize_out)
> +{
> +  return grub_efi_get_variable_with_attributes (var, guid, datasize_out, NULL);
> +}
> +
> +void *
> +grub_efi_get_variable_with_attributes (const char *var,
> +                                       const grub_efi_guid_t *guid,
> +                                       grub_size_t *datasize_out,
> +                                       grub_efi_uint32_t *attributes)

Please put grub_efi_get_variable_with_attributes() before
grub_efi_get_variable().

>  {
>    grub_efi_status_t status;
>    grub_efi_uintn_t datasize = 0;
> @@ -260,7 +269,7 @@ grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
>        return NULL;
>      }
>
> -  status = efi_call_5 (r->get_variable, var16, guid, NULL, &datasize, data);
> +  status = efi_call_5 (r->get_variable, var16, guid, attributes, &datasize, data);
>    grub_free (var16);
>
>    if (status == GRUB_EFI_SUCCESS)
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index 2c6648d46..727722797 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -77,6 +77,10 @@ grub_err_t EXPORT_FUNC (grub_efi_set_virtual_address_map) (grub_efi_uintn_t memo
>  void *EXPORT_FUNC (grub_efi_get_variable) (const char *variable,
>  					   const grub_efi_guid_t *guid,
>  					   grub_size_t *datasize_out);
> +void *EXPORT_FUNC (grub_efi_get_variable_with_attributes) (const char *variable,
> +                                                           const grub_efi_guid_t *guid,
> +                                                           grub_size_t *datasize_out,
> +                                                           grub_efi_uint32_t *attributes);

Same here.

If you do everything which I have asked for you can add
  Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to the Linux kernel
  2018-10-17 18:01     ` [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to the Linux kernel Ignat Korchagin
@ 2018-10-18  9:53       ` Daniel Kiper
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Kiper @ 2018-10-18  9:53 UTC (permalink / raw)
  To: Ignat Korchagin; +Cc: grub-devel

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 <ignat@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.kiper@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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-10-18  9:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-09 16:04 [PATCH] loader/i386/linux: report UEFI secure boot status to the Linux kernel Ignat Korchagin
2018-10-17 14:59 ` Daniel Kiper
2018-10-17 18:01   ` [PATCH v2 0/2] " Ignat Korchagin
2018-10-17 18:01     ` [PATCH v2 1/2] efi: add function to read EFI variables with attributes Ignat Korchagin
2018-10-18  8:48       ` Daniel Kiper
2018-10-17 18:01     ` [PATCH v2 2/2] loader/i386/linux: report UEFI secure boot status to the Linux kernel Ignat Korchagin
2018-10-18  9:53       ` Daniel Kiper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.