Re: [GRUB PATCH RFC 12/18] i386/efi: Report UEFI Secure Boot status to the Linux kernel

2020-05-07 Thread Daniel Kiper
On Wed, May 06, 2020 at 11:36:49AM -0700, Matthew Garrett wrote:
> On Wed, May 6, 2020 at 6:33 AM Daniel Kiper  wrote:
> >
> > On Tue, May 05, 2020 at 10:29:05AM -0700, Matthew Garrett wrote:
> > > On Mon, May 4, 2020 at 4:25 PM Daniel Kiper  
> > > wrote:
> > > >
> > > > Otherwise the kernel does not know its state and cannot enable various
> > > > security features depending on UEFI Secure Boot.
> > >
> > > I think this needs more context. If the kernel is loaded via the EFI
> > > boot stub, the kernel is aware of the UEFI secure boot state. Why
> > > duplicate this functionality in order to avoid the EFI stub?
> >
> > It seems to me that this issue was discussed here [1] and here [2].
> > So, if you want me to improve the commit message I am OK with that.
>
> Yes, I think just providing an explanation for why it's currently
> necessary for you to duplicate this is reasonable.

Sure, will do!

Daniel


Re: [GRUB PATCH RFC 12/18] i386/efi: Report UEFI Secure Boot status to the Linux kernel

2020-05-06 Thread Matthew Garrett
On Wed, May 6, 2020 at 6:33 AM Daniel Kiper  wrote:
>
> On Tue, May 05, 2020 at 10:29:05AM -0700, Matthew Garrett wrote:
> > On Mon, May 4, 2020 at 4:25 PM Daniel Kiper  wrote:
> > >
> > > Otherwise the kernel does not know its state and cannot enable various
> > > security features depending on UEFI Secure Boot.
> >
> > I think this needs more context. If the kernel is loaded via the EFI
> > boot stub, the kernel is aware of the UEFI secure boot state. Why
> > duplicate this functionality in order to avoid the EFI stub?
>
> It seems to me that this issue was discussed here [1] and here [2].
> So, if you want me to improve the commit message I am OK with that.

Yes, I think just providing an explanation for why it's currently
necessary for you to duplicate this is reasonable.


Re: [GRUB PATCH RFC 12/18] i386/efi: Report UEFI Secure Boot status to the Linux kernel

2020-05-06 Thread Daniel Kiper
On Tue, May 05, 2020 at 10:29:05AM -0700, Matthew Garrett wrote:
> On Mon, May 4, 2020 at 4:25 PM Daniel Kiper  wrote:
> >
> > Otherwise the kernel does not know its state and cannot enable various
> > security features depending on UEFI Secure Boot.
>
> I think this needs more context. If the kernel is loaded via the EFI
> boot stub, the kernel is aware of the UEFI secure boot state. Why
> duplicate this functionality in order to avoid the EFI stub?

It seems to me that this issue was discussed here [1] and here [2].
So, if you want me to improve the commit message I am OK with that.

Additionally, FYI I am not happy with that patch too. So, if somebody
has better idea how to do that then I am happy to discuss it here.

Daniel

[1] https://lkml.org/lkml/2020/3/25/982
[2] https://lkml.org/lkml/2020/3/26/985


Re: [GRUB PATCH RFC 12/18] i386/efi: Report UEFI Secure Boot status to the Linux kernel

2020-05-05 Thread Matthew Garrett
On Mon, May 4, 2020 at 4:25 PM Daniel Kiper  wrote:
>
> Otherwise the kernel does not know its state and cannot enable various
> security features depending on UEFI Secure Boot.

I think this needs more context. If the kernel is loaded via the EFI
boot stub, the kernel is aware of the UEFI secure boot state. Why
duplicate this functionality in order to avoid the EFI stub?


[GRUB PATCH RFC 12/18] i386/efi: Report UEFI Secure Boot status to the Linux kernel

2020-05-04 Thread Daniel Kiper
Otherwise the kernel does not know its state and cannot enable various
security features depending on UEFI Secure Boot.

Signed-off-by: Ignat Korchagin 
Signed-off-by: Daniel Kiper 
---
 grub-core/loader/i386/linux.c | 86 ++-
 include/grub/i386/linux.h | 14 ++-
 2 files changed, 97 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index ac1fae72e..952eb1191 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -397,6 +397,87 @@ grub_linux_boot_mmap_fill (grub_uint64_t addr, 
grub_uint64_t size,
   return 0;
 }
 
+#ifdef GRUB_MACHINE_EFI
+/*
+ * Determine whether we're in secure boot mode.
+ *
+ * Please keep the logic in sync with the Linux kernel,
+ * drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot().
+ */
+static grub_uint8_t
+grub_efi_get_secureboot (void)
+{
+  grub_efi_guid_t efi_variable_guid = GRUB_EFI_GLOBAL_VARIABLE_GUID;
+  grub_efi_guid_t efi_shim_lock_guid = GRUB_EFI_SHIM_LOCK_GUID;
+  grub_efi_status_t status;
+  grub_efi_uint32_t attr = 0;
+  grub_size_t size = 0;
+  grub_uint8_t *secboot = NULL;
+  grub_uint8_t *setupmode = NULL;
+  grub_uint8_t *moksbstate = NULL;
+  grub_uint8_t secureboot = GRUB_LINUX_EFI_SECUREBOOT_MODE_UNKNOWN;
+  const char *secureboot_str = "UNKNOWN";
+
+  status = grub_efi_get_variable ("SecureBoot", _variable_guid,
+ , (void **) );
+
+  if (status == GRUB_EFI_NOT_FOUND)
+{
+  secureboot = GRUB_LINUX_EFI_SECUREBOOT_MODE_DISABLED;
+  goto out;
+}
+
+  if (status != GRUB_EFI_SUCCESS)
+goto out;
+
+  status = grub_efi_get_variable ("SetupMode", _variable_guid,
+ , (void **) );
+
+  if (status != GRUB_EFI_SUCCESS)
+goto out;
+
+  if ((*secboot == 0) || (*setupmode == 1))
+{
+  secureboot = GRUB_LINUX_EFI_SECUREBOOT_MODE_DISABLED;
+  goto out;
+}
+
+  /*
+   * See if a user has put the shim into insecure mode. If so, and if the
+   * variable doesn't have the runtime attribute set, we might as well
+   * honor that.
+   */
+  status = grub_efi_get_variable_with_attributes ("MokSBState", 
_shim_lock_guid,
+ , (void **) , 
);
+
+  /* If it fails, we don't care why. Default to secure. */
+  if (status != GRUB_EFI_SUCCESS)
+{
+  secureboot = GRUB_LINUX_EFI_SECUREBOOT_MODE_ENABLED;
+  goto out;
+}
+
+  if (!(attr & GRUB_EFI_VARIABLE_RUNTIME_ACCESS) && *moksbstate == 1)
+secureboot = GRUB_LINUX_EFI_SECUREBOOT_MODE_DISABLED;
+
+  secureboot = GRUB_LINUX_EFI_SECUREBOOT_MODE_ENABLED;
+
+ out:
+  grub_free (moksbstate);
+  grub_free (setupmode);
+  grub_free (secboot);
+
+  if (secureboot == GRUB_LINUX_EFI_SECUREBOOT_MODE_DISABLED)
+secureboot_str = "Disabled";
+  else if (secureboot == GRUB_LINUX_EFI_SECUREBOOT_MODE_ENABLED)
+secureboot_str = "Enabled";
+
+  grub_dprintf ("linux", "UEFI Secure Boot state: %s\n", secureboot_str);
+
+  return secureboot;
+}
+#endif
+
 static grub_err_t
 grub_linux_boot (void)
 {
@@ -579,6 +660,9 @@ 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_get_secureboot ();
+
 err = grub_efi_finish_boot_services (_mmap_size, efi_mmap_buf, NULL,
 _desc_size, _desc_version);
 if (err)
@@ -790,7 +874,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;
   linux_params.type_of_loader = GRUB_LINUX_BOOT_LOADER_TYPE;
 
   /* These two are used (instead of cmd_line_ptr) by older versions of Linux,
diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
index ce30e7fb0..6aea73ddb 100644
--- a/include/grub/i386/linux.h
+++ b/include/grub/i386/linux.h
@@ -49,6 +49,12 @@
 /* Maximum number of MBR signatures to store. */
 #define EDD_MBR_SIG_MAX16
 
+/* Possible values for Linux secure_boot kernel parameter. */
+#define GRUB_LINUX_EFI_SECUREBOOT_MODE_UNSET   0
+#define GRUB_LINUX_EFI_SECUREBOOT_MODE_UNKNOWN 1
+#define GRUB_LINUX_EFI_SECUREBOOT_MODE_DISABLED2
+#define GRUB_LINUX_EFI_SECUREBOOT_MODE_ENABLED 3
+
 #ifdef __x86_64__
 
 #define GRUB_LINUX_EFI_SIGNATURE   \
@@ -275,7 +281,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];
 
   /* Linux setup header copy - BEGIN. */
   grub_uint8_t setup_sects;/* The size of the setup in sectors */
@@