[PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[David Howells]

Here's a set of patches to institute a "locked-down mode" in the kernel and
to set that mode if the kernel is booted in secure-boot mode.  This can be
enabled with CONFIG_LOCK_DOWN_KERNEL.  If a kernel is locked down, the
lockdown can be lifted by typing SysRq+x on a keyboard attached to the
machine if CONFIG_EFI_ALLOW_SECURE_BOOT_EXIT is enabled.  The exact key can
be configured as 'x' is already taken on some arches.

Inside the kernel, kernel_is_locked_down() is used to check if the kernel
is in lockdown mode.  In lock-down mode, at least the following
restrictions will need to be emplaced:

 (1) No unsigned modules, kexec images or firmware.

 (2) No direct read/write access of the kernel image.  (Shouldn't be able
     to modify it and shouldn't be able to read out crypto data).

 (3) No direct access to devices.  (DMA could be used to access/modify the
     kernel image).

 (4) No manual setting of device register addresses to cause a driver for
     one device to mess around with another device, thereby permitting DMA.

 (5) No storage of unencrypted kernel image to disk (no suspend-to-disk
     without hardware support).

I have patches pending that effect most of the above.  However, the
firmware signature checking is being handled by someone else.  Further, it
has come to light recently that debugfs needs attention, so that isn't done
yet.

Note that the secure boot mode entry doesn't currently work if the kernel
is booted from current i386/x86_64 Grub as there's a bug in Grub whereby it
doesn't initialise the boot_params correctly.  The incorrect initialisation
causes sanitize_boot_params() to be triggered, thereby zapping the secure
boot flag determined by the EFI boot wrapper.

David
---
David Howells (3):
      efi: Move the x86 secure boot switch to generic code
      Add the ability to lock down access to the running kernel image
      efi: Lock down the kernel if booted in secure boot mode

Josh Boyer (1):
      efi: Add EFI_SECURE_BOOT bit

Kyle McMartin (1):
      Add a sysrq option to exit secure boot mode


 arch/x86/include/asm/efi.h        |    2 +
 arch/x86/kernel/setup.c           |   14 ------
 drivers/firmware/efi/Kconfig      |   34 ++++++++++++++++
 drivers/firmware/efi/Makefile     |    1 
 drivers/firmware/efi/secureboot.c |   80 +++++++++++++++++++++++++++++++++++++
 drivers/input/misc/uinput.c       |    1 
 drivers/tty/sysrq.c               |   19 ++++++---
 include/linux/efi.h               |    7 +++
 include/linux/input.h             |    5 ++
 include/linux/kernel.h            |    9 ++++
 include/linux/security.h          |   11 +++++
 include/linux/sysrq.h             |    8 +++-
 kernel/debug/kdb/kdb_main.c       |    2 -
 security/Kconfig                  |   15 +++++++
 security/Makefile                 |    3 +
 security/lock_down.c              |   46 +++++++++++++++++++++
 16 files changed, 236 insertions(+), 21 deletions(-)
 create mode 100644 drivers/firmware/efi/secureboot.c
 create mode 100644 security/lock_down.c

[PATCH 1/5] efi: Move the x86 secure boot switch to generic code

[David Howells]

Move the switch-statement in x86's setup_arch() that inteprets the
secure_boot boot parameter to generic code.

Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/kernel/setup.c           |   14 +-------------
 drivers/firmware/efi/Kconfig      |   23 +++++++++++++++++++++++
 drivers/firmware/efi/Makefile     |    1 +
 drivers/firmware/efi/secureboot.c |   34 ++++++++++++++++++++++++++++++++++
 include/linux/efi.h               |    6 ++++++
 5 files changed, 65 insertions(+), 13 deletions(-)
 create mode 100644 drivers/firmware/efi/secureboot.c

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0b4d3c686b1e..8bffbd8d2c1c 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1177,19 +1177,7 @@ void __init setup_arch(char **cmdline_p)
 	/* Allocate bigger log buffer */
 	setup_log_buf(1);
 
-	if (efi_enabled(EFI_BOOT)) {
-		switch (boot_params.secure_boot) {
-		case efi_secureboot_mode_disabled:
-			pr_info("Secure boot disabled\n");
-			break;
-		case efi_secureboot_mode_enabled:
-			pr_info("Secure boot enabled\n");
-			break;
-		default:
-			pr_info("Secure boot could not be determined\n");
-			break;
-		}
-	}
+	efi_set_secure_boot(boot_params.secure_boot);
 
 	reserve_initrd();
 
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 394db40ed374..c40fdeaf9a45 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -84,6 +84,29 @@ config EFI_PARAMS_FROM_FDT
 config EFI_RUNTIME_WRAPPERS
 	bool
 
+config EFI_SECURE_BOOT
+	bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
+	default n
+	help
+	  UEFI Secure Boot provides a mechanism for ensuring that the firmware
+	  will only load signed bootloaders and kernels.  Secure boot mode may
+	  be determined from EFI variables provided by the system firmware if
+	  not indicated by the boot parameters.
+
+	  Enabling this option turns on support for UEFI secure boot in the
+	  kernel.  This will result in various kernel facilities being locked
+	  away from userspace if the kernel detects that it has been booted in
+	  secure boot mode.  If it hasn't been booted in secure boot mode, or
+	  this cannot be determined, the lock down doesn't occur.
+
+	  The kernel facilities that get locked down include:
+	  - Viewing or changing the kernel's memory
+	  - Directly accessing ioports
+	  - Directly specifying ioports and other hardware parameters to drivers
+	  - Storing the kernel image unencrypted for hibernation
+	  - Loading unsigned modules
+	  - Kexec'ing unsigned images
+
 config EFI_ARMSTUB
 	bool
 
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 0329d319d89a..9dfd8530063f 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
 obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
 obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
+obj-$(CONFIG_EFI_SECURE_BOOT)		+= secureboot.o
 obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
 
 arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
diff --git a/drivers/firmware/efi/secureboot.c b/drivers/firmware/efi/secureboot.c
new file mode 100644
index 000000000000..cf5bccae15e8
--- /dev/null
+++ b/drivers/firmware/efi/secureboot.c
@@ -0,0 +1,34 @@
+/* Core kernel secure boot support.
+ *
+ * Copyright (C) 2017 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#include <linux/efi.h>
+#include <linux/kernel.h>
+#include <linux/printk.h>
+
+/*
+ * Decide what to do when UEFI secure boot mode is enabled.
+ */
+void __init efi_set_secure_boot(enum efi_secureboot_mode mode)
+{
+	if (efi_enabled(EFI_BOOT)) {
+		switch (mode) {
+		case efi_secureboot_mode_disabled:
+			pr_info("Secure boot disabled\n");
+			break;
+		case efi_secureboot_mode_enabled:
+			pr_info("Secure boot enabled\n");
+			break;
+		default:
+			pr_info("Secure boot could not be determined\n");
+			break;
+		}
+	}
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8269bcb8ccf7..e2f53edccf15 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1497,6 +1497,12 @@ enum efi_secureboot_mode {
 };
 enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
 
+#ifdef CONFIG_EFI_SECURE_BOOT
+void __init efi_set_secure_boot(enum efi_secureboot_mode mode);
+#else
+static inline void efi_set_secure_boot(enum efi_secureboot_mode mode) {}
+#endif
+
 /*
  * Arch code can implement the following three template macros, avoiding
  * reptition for the void/non-void return cases of {__,}efi_call_virt():

Re: [PATCH 1/5] efi: Move the x86 secure boot switch to generic code

[joeyli]

Hi David,

On Wed, May 24, 2017 at 03:45:25PM +0100, David Howells wrote:
> Move the switch-statement in x86's setup_arch() that inteprets the
> secure_boot boot parameter to generic code.
> 
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: David Howells <dhowells@redhat.com>

I reviewed the context for this patch.

Reviewed-by: Joey Lee <jlee@suse.com>

Regards
Joey Lee

> ---
> 
>  arch/x86/kernel/setup.c           |   14 +-------------
>  drivers/firmware/efi/Kconfig      |   23 +++++++++++++++++++++++
>  drivers/firmware/efi/Makefile     |    1 +
>  drivers/firmware/efi/secureboot.c |   34 ++++++++++++++++++++++++++++++++++
>  include/linux/efi.h               |    6 ++++++
>  5 files changed, 65 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/firmware/efi/secureboot.c
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 0b4d3c686b1e..8bffbd8d2c1c 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1177,19 +1177,7 @@ void __init setup_arch(char **cmdline_p)
>  	/* Allocate bigger log buffer */
>  	setup_log_buf(1);
>  
> -	if (efi_enabled(EFI_BOOT)) {
> -		switch (boot_params.secure_boot) {
> -		case efi_secureboot_mode_disabled:
> -			pr_info("Secure boot disabled\n");
> -			break;
> -		case efi_secureboot_mode_enabled:
> -			pr_info("Secure boot enabled\n");
> -			break;
> -		default:
> -			pr_info("Secure boot could not be determined\n");
> -			break;
> -		}
> -	}
> +	efi_set_secure_boot(boot_params.secure_boot);
>  
>  	reserve_initrd();
>  
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 394db40ed374..c40fdeaf9a45 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -84,6 +84,29 @@ config EFI_PARAMS_FROM_FDT
>  config EFI_RUNTIME_WRAPPERS
>  	bool
>  
> +config EFI_SECURE_BOOT
> +	bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
> +	default n
> +	help
> +	  UEFI Secure Boot provides a mechanism for ensuring that the firmware
> +	  will only load signed bootloaders and kernels.  Secure boot mode may
> +	  be determined from EFI variables provided by the system firmware if
> +	  not indicated by the boot parameters.
> +
> +	  Enabling this option turns on support for UEFI secure boot in the
> +	  kernel.  This will result in various kernel facilities being locked
> +	  away from userspace if the kernel detects that it has been booted in
> +	  secure boot mode.  If it hasn't been booted in secure boot mode, or
> +	  this cannot be determined, the lock down doesn't occur.
> +
> +	  The kernel facilities that get locked down include:
> +	  - Viewing or changing the kernel's memory
> +	  - Directly accessing ioports
> +	  - Directly specifying ioports and other hardware parameters to drivers
> +	  - Storing the kernel image unencrypted for hibernation
> +	  - Loading unsigned modules
> +	  - Kexec'ing unsigned images
> +
>  config EFI_ARMSTUB
>  	bool
>  
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 0329d319d89a..9dfd8530063f 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_mem.o
>  obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
>  obj-$(CONFIG_EFI_TEST)			+= test/
>  obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
> +obj-$(CONFIG_EFI_SECURE_BOOT)		+= secureboot.o
>  obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
>  
>  arm-obj-$(CONFIG_EFI)			:= arm-init.o arm-runtime.o
> diff --git a/drivers/firmware/efi/secureboot.c b/drivers/firmware/efi/secureboot.c
> new file mode 100644
> index 000000000000..cf5bccae15e8
> --- /dev/null
> +++ b/drivers/firmware/efi/secureboot.c
> @@ -0,0 +1,34 @@
> +/* Core kernel secure boot support.
> + *
> + * Copyright (C) 2017 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/kernel.h>
> +#include <linux/printk.h>
> +
> +/*
> + * Decide what to do when UEFI secure boot mode is enabled.
> + */
> +void __init efi_set_secure_boot(enum efi_secureboot_mode mode)
> +{
> +	if (efi_enabled(EFI_BOOT)) {
> +		switch (mode) {
> +		case efi_secureboot_mode_disabled:
> +			pr_info("Secure boot disabled\n");
> +			break;
> +		case efi_secureboot_mode_enabled:
> +			pr_info("Secure boot enabled\n");
> +			break;
> +		default:
> +			pr_info("Secure boot could not be determined\n");
> +			break;
> +		}
> +	}
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 8269bcb8ccf7..e2f53edccf15 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1497,6 +1497,12 @@ enum efi_secureboot_mode {
>  };
>  enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
>  
> +#ifdef CONFIG_EFI_SECURE_BOOT
> +void __init efi_set_secure_boot(enum efi_secureboot_mode mode);
> +#else
> +static inline void efi_set_secure_boot(enum efi_secureboot_mode mode) {}
> +#endif
> +
>  /*
>   * Arch code can implement the following three template macros, avoiding
>   * reptition for the void/non-void return cases of {__,}efi_call_virt():
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] efi: Add EFI_SECURE_BOOT bit

[David Howells]

From: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>

UEFI machines can be booted in Secure Boot mode.  Add a EFI_SECURE_BOOT bit
that can be passed to efi_enabled() to find out whether secure boot is
enabled.

This will be used by the SysRq+x handler, registered by the x86 arch, to
find out whether secure boot mode is enabled so that it can be disabled.

Signed-off-by: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---

 drivers/firmware/efi/secureboot.c |    1 +
 include/linux/efi.h               |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/firmware/efi/secureboot.c b/drivers/firmware/efi/secureboot.c
index cf5bccae15e8..730518061a14 100644
--- a/drivers/firmware/efi/secureboot.c
+++ b/drivers/firmware/efi/secureboot.c
@@ -24,6 +24,7 @@ void __init efi_set_secure_boot(enum efi_secureboot_mode mode)
 			pr_info("Secure boot disabled\n");
 			break;
 		case efi_secureboot_mode_enabled:
+			set_bit(EFI_SECURE_BOOT, &efi.flags);
 			pr_info("Secure boot enabled\n");
 			break;
 		default:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e2f53edccf15..5e9a2d7df089 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1081,6 +1081,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_DBG			8	/* Print additional debug info at runtime */
 #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
 #define EFI_MEM_ATTR		10	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
+#define EFI_SECURE_BOOT		11	/* Are we in Secure Boot mode? */
 
 #ifdef CONFIG_EFI
 /*

Re: [PATCH 2/5] efi: Add EFI_SECURE_BOOT bit

[joeyli]

On Wed, May 24, 2017 at 03:45:32PM +0100, David Howells wrote:
> From: Josh Boyer <jwboyer@fedoraproject.org>
> 
> UEFI machines can be booted in Secure Boot mode.  Add a EFI_SECURE_BOOT bit
> that can be passed to efi_enabled() to find out whether secure boot is
> enabled.
> 
> This will be used by the SysRq+x handler, registered by the x86 arch, to
> find out whether secure boot mode is enabled so that it can be disabled.
> 
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linux-efi@vger.kernel.org

Reviewed-by: Joey Lee <jlee@suse.com>

Regards
Joey Lee

> ---
> 
>  drivers/firmware/efi/secureboot.c |    1 +
>  include/linux/efi.h               |    1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/firmware/efi/secureboot.c b/drivers/firmware/efi/secureboot.c
> index cf5bccae15e8..730518061a14 100644
> --- a/drivers/firmware/efi/secureboot.c
> +++ b/drivers/firmware/efi/secureboot.c
> @@ -24,6 +24,7 @@ void __init efi_set_secure_boot(enum efi_secureboot_mode mode)
>  			pr_info("Secure boot disabled\n");
>  			break;
>  		case efi_secureboot_mode_enabled:
> +			set_bit(EFI_SECURE_BOOT, &efi.flags);
>  			pr_info("Secure boot enabled\n");
>  			break;
>  		default:
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index e2f53edccf15..5e9a2d7df089 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1081,6 +1081,7 @@ extern int __init efi_setup_pcdp_console(char *);
>  #define EFI_DBG			8	/* Print additional debug info at runtime */
>  #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
>  #define EFI_MEM_ATTR		10	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
> +#define EFI_SECURE_BOOT		11	/* Are we in Secure Boot mode? */
>  
>  #ifdef CONFIG_EFI
>  /*
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] Add the ability to lock down access to the running kernel image

[David Howells]

Provide a single call to allow kernel code to determine whether the system
should be locked down, thereby disallowing various accesses that might
allow the running kernel image to be changed including the loading of
modules that aren't validly signed with a key we recognise, fiddling with
MSR registers and disallowing hibernation,

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: James Morris <james.l.morris@oracle.com>
---

 include/linux/kernel.h   |    9 +++++++++
 include/linux/security.h |   11 +++++++++++
 security/Kconfig         |   15 +++++++++++++++
 security/Makefile        |    3 +++
 security/lock_down.c     |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+)
 create mode 100644 security/lock_down.c

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 13bc08aba704..282a1684d6e8 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -276,6 +276,15 @@ extern int oops_may_print(void);
 void do_exit(long error_code) __noreturn;
 void complete_and_exit(struct completion *, long) __noreturn;
 
+#ifdef CONFIG_LOCK_DOWN_KERNEL
+extern bool kernel_is_locked_down(void);
+#else
+static inline bool kernel_is_locked_down(void)
+{
+	return false;
+}
+#endif
+
 /* Internal, do not use. */
 int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res);
 int __must_check _kstrtol(const char *s, unsigned int base, long *res);
diff --git a/include/linux/security.h b/include/linux/security.h
index af675b576645..8db2d886aa90 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1698,5 +1698,16 @@ static inline void free_secdata(void *secdata)
 { }
 #endif /* CONFIG_SECURITY */
 
+#ifdef CONFIG_LOCK_DOWN_KERNEL
+extern void __init lock_kernel_down(void);
+#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
+extern void lift_kernel_lockdown(void);
+#endif
+#else
+static inline void lock_kernel_down(void)
+{
+}
+#endif
+
 #endif /* ! __LINUX_SECURITY_H */
 
diff --git a/security/Kconfig b/security/Kconfig
index 93027fdf47d1..4baac4aab277 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -189,6 +189,21 @@ config STATIC_USERMODEHELPER_PATH
 	  If you wish for all usermode helper programs to be disabled,
 	  specify an empty string here (i.e. "").
 
+config LOCK_DOWN_KERNEL
+	bool "Allow the kernel to be 'locked down'"
+	help
+	  Allow the kernel to be locked down under certain circumstances, for
+	  instance if UEFI secure boot is enabled.  Locking down the kernel
+	  turns off various features that might otherwise allow access to the
+	  kernel image (eg. setting MSR registers).
+
+config ALLOW_LOCKDOWN_LIFT
+	bool
+	help
+	  Allow the lockdown on a kernel to be lifted, thereby restoring the
+	  ability of userspace to access the kernel image (eg. by SysRq+x under
+	  x86).
+
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
diff --git a/security/Makefile b/security/Makefile
index f2d71cdb8e19..8c4a43e3d4e0 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -29,3 +29,6 @@ obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
 # Object integrity file lists
 subdir-$(CONFIG_INTEGRITY)		+= integrity
 obj-$(CONFIG_INTEGRITY)			+= integrity/
+
+# Allow the kernel to be locked down
+obj-$(CONFIG_LOCK_DOWN_KERNEL)		+= lock_down.o
diff --git a/security/lock_down.c b/security/lock_down.c
new file mode 100644
index 000000000000..dd98422fbda7
--- /dev/null
+++ b/security/lock_down.c
@@ -0,0 +1,46 @@
+/* Lock down the kernel
+ *
+ * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#include <linux/security.h>
+#include <linux/export.h>
+
+#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
+static __read_mostly bool kernel_locked_down;
+#else
+static __ro_after_init bool kernel_locked_down;
+#endif
+
+/*
+ * Put the kernel into lock-down mode.
+ */
+void __init lock_kernel_down(void)
+{
+	kernel_locked_down = true;
+}
+
+/*
+ * Take the kernel out of lockdown mode.
+ */
+#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
+void lift_kernel_lockdown(void)
+{
+	kernel_locked_down = false;
+}
+#endif
+
+/**
+ * kernel_is_locked_down - Find out if the kernel is locked down
+ */
+bool kernel_is_locked_down(void)
+{
+	return kernel_locked_down;
+}
+EXPORT_SYMBOL(kernel_is_locked_down);

Re: [PATCH 3/5] Add the ability to lock down access to the running kernel image

[David Howells]

Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote:

> > +#ifdef CONFIG_LOCK_DOWN_KERNEL
> > +extern bool kernel_is_locked_down(void);
> > +#else
> > +static inline bool kernel_is_locked_down(void)
> 
> Should this be a bool or an int? I can imagine that someone is going to want
> various different degrees of lock down for kernels. As an int you could
> return a bitmap indicating which features were locked. This would allow
> additional things to be locked down without changing the interface.

At the moment it makes no difference, since the return value is only ever
passed directly to an if-statement.

Also, do you have an idea as to how is should be divided up?

There aren't so many cases, at least not yet, that they can't be fixed up,
perhaps with a coccinelle script.

David

Re: [PATCH 3/5] Add the ability to lock down access to the running kernel image

[Casey Schaufler]

On 5/24/2017 11:53 PM, David Howells wrote:
> Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote:
>
>>> +#ifdef CONFIG_LOCK_DOWN_KERNEL
>>> +extern bool kernel_is_locked_down(void);
>>> +#else
>>> +static inline bool kernel_is_locked_down(void)
>> Should this be a bool or an int? I can imagine that someone is going to want
>> various different degrees of lock down for kernels. As an int you could
>> return a bitmap indicating which features were locked. This would allow
>> additional things to be locked down without changing the interface.
> At the moment it makes no difference, since the return value is only ever
> passed directly to an if-statement.
>
> Also, do you have an idea as to how is should be divided up?

You called out five distinct features in 0/5, so how about
a bit for each of those?

Actually, I don't care which way you go. The current code works
for me. I am just concerned that the granularity fiends might come
around later.


>
> There aren't so many cases, at least not yet, that they can't be fixed up,
> perhaps with a coccinelle script.
>
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/5] Add the ability to lock down access to the running kernel image

[David Howells]

Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote:

> You called out five distinct features in 0/5, so how about
> a bit for each of those?

Actually, there are more than five in that list - there are three in the first
item - and I'm not sure the remaining categories are quite as well defined as
I made it seem.

Also, that sort of categorisation might not be what we actually need: it might
end up coming down to a no-write vs no-read-or-write split instead.

> Actually, I don't care which way you go. The current code works
> for me. I am just concerned that the granularity fiends might come
> around later.

In that case, I'll leave it as is for the moment.  It doesn't introduce so
many calls that they're impossible to change.

David

Re: [PATCH 3/5] Add the ability to lock down access to the running kernel image

[joeyli]

On Fri, May 26, 2017 at 01:43:12PM +0100, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> > You called out five distinct features in 0/5, so how about
> > a bit for each of those?
> 
> Actually, there are more than five in that list - there are three in the first
> item - and I'm not sure the remaining categories are quite as well defined as
> I made it seem.
>

Do we have a public place (e.g. wiki page) to put the list of lock-down
functions?

Thanks a lot!
Joey Lee 

Re: [PATCH 3/5] Add the ability to lock down access to the running kernel image

[Casey Schaufler]

On 5/24/2017 7:45 AM, David Howells wrote:
> Provide a single call to allow kernel code to determine whether the system
> should be locked down, thereby disallowing various accesses that might
> allow the running kernel image to be changed including the loading of
> modules that aren't validly signed with a key we recognise, fiddling with
> MSR registers and disallowing hibernation,
>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>
>  include/linux/kernel.h   |    9 +++++++++
>  include/linux/security.h |   11 +++++++++++
>  security/Kconfig         |   15 +++++++++++++++
>  security/Makefile        |    3 +++
>  security/lock_down.c     |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 84 insertions(+)
>  create mode 100644 security/lock_down.c
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 13bc08aba704..282a1684d6e8 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -276,6 +276,15 @@ extern int oops_may_print(void);
>  void do_exit(long error_code) __noreturn;
>  void complete_and_exit(struct completion *, long) __noreturn;
>  
> +#ifdef CONFIG_LOCK_DOWN_KERNEL
> +extern bool kernel_is_locked_down(void);
> +#else
> +static inline bool kernel_is_locked_down(void)

Should this be a bool or an int? I can imagine that
someone is going to want various different degrees
of lock down for kernels. As an int you could return
a bitmap indicating which features were locked. This
would allow additional things to be locked down
without changing the interface.

> +{
> +	return false;
> +}
> +#endif
> +
>  /* Internal, do not use. */
>  int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res);
>  int __must_check _kstrtol(const char *s, unsigned int base, long *res);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index af675b576645..8db2d886aa90 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1698,5 +1698,16 @@ static inline void free_secdata(void *secdata)
>  { }
>  #endif /* CONFIG_SECURITY */
>  
> +#ifdef CONFIG_LOCK_DOWN_KERNEL
> +extern void __init lock_kernel_down(void);
> +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
> +extern void lift_kernel_lockdown(void);
> +#endif
> +#else
> +static inline void lock_kernel_down(void)
> +{
> +}
> +#endif
> +
>  #endif /* ! __LINUX_SECURITY_H */
>  
> diff --git a/security/Kconfig b/security/Kconfig
> index 93027fdf47d1..4baac4aab277 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -189,6 +189,21 @@ config STATIC_USERMODEHELPER_PATH
>  	  If you wish for all usermode helper programs to be disabled,
>  	  specify an empty string here (i.e. "").
>  
> +config LOCK_DOWN_KERNEL
> +	bool "Allow the kernel to be 'locked down'"
> +	help
> +	  Allow the kernel to be locked down under certain circumstances, for
> +	  instance if UEFI secure boot is enabled.  Locking down the kernel
> +	  turns off various features that might otherwise allow access to the
> +	  kernel image (eg. setting MSR registers).
> +
> +config ALLOW_LOCKDOWN_LIFT
> +	bool
> +	help
> +	  Allow the lockdown on a kernel to be lifted, thereby restoring the
> +	  ability of userspace to access the kernel image (eg. by SysRq+x under
> +	  x86).
> +
>  source security/selinux/Kconfig
>  source security/smack/Kconfig
>  source security/tomoyo/Kconfig
> diff --git a/security/Makefile b/security/Makefile
> index f2d71cdb8e19..8c4a43e3d4e0 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -29,3 +29,6 @@ obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
>  # Object integrity file lists
>  subdir-$(CONFIG_INTEGRITY)		+= integrity
>  obj-$(CONFIG_INTEGRITY)			+= integrity/
> +
> +# Allow the kernel to be locked down
> +obj-$(CONFIG_LOCK_DOWN_KERNEL)		+= lock_down.o
> diff --git a/security/lock_down.c b/security/lock_down.c
> new file mode 100644
> index 000000000000..dd98422fbda7
> --- /dev/null
> +++ b/security/lock_down.c
> @@ -0,0 +1,46 @@
> +/* Lock down the kernel
> + *
> + * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +#include <linux/security.h>
> +#include <linux/export.h>
> +
> +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
> +static __read_mostly bool kernel_locked_down;
> +#else
> +static __ro_after_init bool kernel_locked_down;
> +#endif
> +
> +/*
> + * Put the kernel into lock-down mode.
> + */
> +void __init lock_kernel_down(void)
> +{
> +	kernel_locked_down = true;
> +}
> +
> +/*
> + * Take the kernel out of lockdown mode.
> + */
> +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
> +void lift_kernel_lockdown(void)
> +{
> +	kernel_locked_down = false;
> +}
> +#endif
> +
> +/**
> + * kernel_is_locked_down - Find out if the kernel is locked down
> + */
> +bool kernel_is_locked_down(void)
> +{
> +	return kernel_locked_down;
> +}
> +EXPORT_SYMBOL(kernel_is_locked_down);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/5] Add the ability to lock down access to the running kernel image

[joeyli]

On Wed, May 24, 2017 at 03:45:45PM +0100, David Howells wrote:
> Provide a single call to allow kernel code to determine whether the system
> should be locked down, thereby disallowing various accesses that might
> allow the running kernel image to be changed including the loading of
> modules that aren't validly signed with a key we recognise, fiddling with
> MSR registers and disallowing hibernation,
> 
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

Reviewed-by: Joey Lee <jlee-IBi9RG/b67k@public.gmane.org>

Regards
Joey Lee

> ---
> 
>  include/linux/kernel.h   |    9 +++++++++
>  include/linux/security.h |   11 +++++++++++
>  security/Kconfig         |   15 +++++++++++++++
>  security/Makefile        |    3 +++
>  security/lock_down.c     |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 84 insertions(+)
>  create mode 100644 security/lock_down.c
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 13bc08aba704..282a1684d6e8 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -276,6 +276,15 @@ extern int oops_may_print(void);
>  void do_exit(long error_code) __noreturn;
>  void complete_and_exit(struct completion *, long) __noreturn;
>  
> +#ifdef CONFIG_LOCK_DOWN_KERNEL
> +extern bool kernel_is_locked_down(void);
> +#else
> +static inline bool kernel_is_locked_down(void)
> +{
> +	return false;
> +}
> +#endif
> +
>  /* Internal, do not use. */
>  int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res);
>  int __must_check _kstrtol(const char *s, unsigned int base, long *res);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index af675b576645..8db2d886aa90 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1698,5 +1698,16 @@ static inline void free_secdata(void *secdata)
>  { }
>  #endif /* CONFIG_SECURITY */
>  
> +#ifdef CONFIG_LOCK_DOWN_KERNEL
> +extern void __init lock_kernel_down(void);
> +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
> +extern void lift_kernel_lockdown(void);
> +#endif
> +#else
> +static inline void lock_kernel_down(void)
> +{
> +}
> +#endif
> +
>  #endif /* ! __LINUX_SECURITY_H */
>  
> diff --git a/security/Kconfig b/security/Kconfig
> index 93027fdf47d1..4baac4aab277 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -189,6 +189,21 @@ config STATIC_USERMODEHELPER_PATH
>  	  If you wish for all usermode helper programs to be disabled,
>  	  specify an empty string here (i.e. "").
>  
> +config LOCK_DOWN_KERNEL
> +	bool "Allow the kernel to be 'locked down'"
> +	help
> +	  Allow the kernel to be locked down under certain circumstances, for
> +	  instance if UEFI secure boot is enabled.  Locking down the kernel
> +	  turns off various features that might otherwise allow access to the
> +	  kernel image (eg. setting MSR registers).
> +
> +config ALLOW_LOCKDOWN_LIFT
> +	bool
> +	help
> +	  Allow the lockdown on a kernel to be lifted, thereby restoring the
> +	  ability of userspace to access the kernel image (eg. by SysRq+x under
> +	  x86).
> +
>  source security/selinux/Kconfig
>  source security/smack/Kconfig
>  source security/tomoyo/Kconfig
> diff --git a/security/Makefile b/security/Makefile
> index f2d71cdb8e19..8c4a43e3d4e0 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -29,3 +29,6 @@ obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
>  # Object integrity file lists
>  subdir-$(CONFIG_INTEGRITY)		+= integrity
>  obj-$(CONFIG_INTEGRITY)			+= integrity/
> +
> +# Allow the kernel to be locked down
> +obj-$(CONFIG_LOCK_DOWN_KERNEL)		+= lock_down.o
> diff --git a/security/lock_down.c b/security/lock_down.c
> new file mode 100644
> index 000000000000..dd98422fbda7
> --- /dev/null
> +++ b/security/lock_down.c
> @@ -0,0 +1,46 @@
> +/* Lock down the kernel
> + *
> + * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +#include <linux/security.h>
> +#include <linux/export.h>
> +
> +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
> +static __read_mostly bool kernel_locked_down;
> +#else
> +static __ro_after_init bool kernel_locked_down;
> +#endif
> +
> +/*
> + * Put the kernel into lock-down mode.
> + */
> +void __init lock_kernel_down(void)
> +{
> +	kernel_locked_down = true;
> +}
> +
> +/*
> + * Take the kernel out of lockdown mode.
> + */
> +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
> +void lift_kernel_lockdown(void)
> +{
> +	kernel_locked_down = false;
> +}
> +#endif
> +
> +/**
> + * kernel_is_locked_down - Find out if the kernel is locked down
> + */
> +bool kernel_is_locked_down(void)
> +{
> +	return kernel_locked_down;
> +}
> +EXPORT_SYMBOL(kernel_is_locked_down);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] efi: Lock down the kernel if booted in secure boot mode

[David Howells]

UEFI Secure Boot provides a mechanism for ensuring that the firmware will
only load signed bootloaders and kernels.  Certain use cases may also
require that all kernel modules also be signed.  Add a configuration option
that to lock down the kernel - which includes requiring validly signed
modules - if the kernel is secure-booted.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-efi@vger.kernel.org
---

 drivers/firmware/efi/Kconfig      |    1 +
 drivers/firmware/efi/secureboot.c |   10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index c40fdeaf9a45..d03af2d5f52f 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -87,6 +87,7 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_SECURE_BOOT
 	bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
 	default n
+	select LOCK_DOWN_KERNEL
 	help
 	  UEFI Secure Boot provides a mechanism for ensuring that the firmware
 	  will only load signed bootloaders and kernels.  Secure boot mode may
diff --git a/drivers/firmware/efi/secureboot.c b/drivers/firmware/efi/secureboot.c
index 730518061a14..7292a3b832e3 100644
--- a/drivers/firmware/efi/secureboot.c
+++ b/drivers/firmware/efi/secureboot.c
@@ -12,6 +12,7 @@
 #include <linux/efi.h>
 #include <linux/kernel.h>
 #include <linux/printk.h>
+#include <linux/security.h>
 
 /*
  * Decide what to do when UEFI secure boot mode is enabled.
@@ -23,10 +24,17 @@ void __init efi_set_secure_boot(enum efi_secureboot_mode mode)
 		case efi_secureboot_mode_disabled:
 			pr_info("Secure boot disabled\n");
 			break;
+
 		case efi_secureboot_mode_enabled:
 			set_bit(EFI_SECURE_BOOT, &efi.flags);
-			pr_info("Secure boot enabled\n");
+			if (IS_ENABLED(CONFIG_LOCK_DOWN_KERNEL)) {
+				lock_kernel_down();
+				pr_info("Secure boot enabled and kernel locked down\n");
+			} else {
+				pr_info("Secure boot enabled\n");
+			}
 			break;
+
 		default:
 			pr_info("Secure boot could not be determined\n");
 			break;

Re: [PATCH 4/5] efi: Lock down the kernel if booted in secure boot mode

[joeyli]

On Wed, May 24, 2017 at 03:45:56PM +0100, David Howells wrote:
> UEFI Secure Boot provides a mechanism for ensuring that the firmware will
> only load signed bootloaders and kernels.  Certain use cases may also
> require that all kernel modules also be signed.  Add a configuration option
> that to lock down the kernel - which includes requiring validly signed
> modules - if the kernel is secure-booted.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linux-efi@vger.kernel.org

Reviewed-by: Joey Lee <jlee@suse.com>

Regards
Joey Lee

> ---
> 
>  drivers/firmware/efi/Kconfig      |    1 +
>  drivers/firmware/efi/secureboot.c |   10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index c40fdeaf9a45..d03af2d5f52f 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -87,6 +87,7 @@ config EFI_RUNTIME_WRAPPERS
>  config EFI_SECURE_BOOT
>  	bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
>  	default n
> +	select LOCK_DOWN_KERNEL
>  	help
>  	  UEFI Secure Boot provides a mechanism for ensuring that the firmware
>  	  will only load signed bootloaders and kernels.  Secure boot mode may
> diff --git a/drivers/firmware/efi/secureboot.c b/drivers/firmware/efi/secureboot.c
> index 730518061a14..7292a3b832e3 100644
> --- a/drivers/firmware/efi/secureboot.c
> +++ b/drivers/firmware/efi/secureboot.c
> @@ -12,6 +12,7 @@
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
>  #include <linux/printk.h>
> +#include <linux/security.h>
>  
>  /*
>   * Decide what to do when UEFI secure boot mode is enabled.
> @@ -23,10 +24,17 @@ void __init efi_set_secure_boot(enum efi_secureboot_mode mode)
>  		case efi_secureboot_mode_disabled:
>  			pr_info("Secure boot disabled\n");
>  			break;
> +
>  		case efi_secureboot_mode_enabled:
>  			set_bit(EFI_SECURE_BOOT, &efi.flags);
> -			pr_info("Secure boot enabled\n");
> +			if (IS_ENABLED(CONFIG_LOCK_DOWN_KERNEL)) {
> +				lock_kernel_down();
> +				pr_info("Secure boot enabled and kernel locked down\n");
> +			} else {
> +				pr_info("Secure boot enabled\n");
> +			}
>  			break;
> +
>  		default:
>  			pr_info("Secure boot could not be determined\n");
>  			break;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] Add a sysrq option to exit secure boot mode

[David Howells]

From: Kyle McMartin <kyle@redhat.com>

Make sysrq+x exit secure boot mode on x86_64, thereby allowing the running
kernel image to be modified.  This lifts the lockdown.

Signed-off-by: Kyle McMartin <kyle@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: x86@kernel.org
---

 arch/x86/include/asm/efi.h        |    2 ++
 drivers/firmware/efi/Kconfig      |   10 ++++++++++
 drivers/firmware/efi/secureboot.c |   37 +++++++++++++++++++++++++++++++++++++
 drivers/input/misc/uinput.c       |    1 +
 drivers/tty/sysrq.c               |   19 +++++++++++++------
 include/linux/input.h             |    5 +++++
 include/linux/sysrq.h             |    8 +++++++-
 kernel/debug/kdb/kdb_main.c       |    2 +-
 8 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 6d74cc3802e6..b4e641ace1bd 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -31,6 +31,8 @@
 
 #define ARCH_EFI_IRQ_FLAGS_MASK	X86_EFLAGS_IF
 
+#define EFI_SECURE_BOOT_EXIT_KEY 'x'
+
 #ifdef CONFIG_X86_32
 
 extern unsigned long asmlinkage efi_call_phys(void *, ...);
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index d03af2d5f52f..bd27f35ba528 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -108,6 +108,16 @@ config EFI_SECURE_BOOT
 	  - Loading unsigned modules
 	  - Kexec'ing unsigned images
 
+config EFI_ALLOW_SECURE_BOOT_EXIT
+	def_bool n
+	depends on EFI_SECURE_BOOT && MAGIC_SYSRQ
+	select ALLOW_LOCKDOWN_LIFT
+	prompt "Allow secure boot mode to be exited with SysRq+x on a keyboard"
+	---help---
+	  Allow secure boot mode to be exited and the kernel lockdown lifted by
+	  typing SysRq+x on a keyboard attached to the system (not permitted
+	  through procfs).
+
 config EFI_ARMSTUB
 	bool
 
diff --git a/drivers/firmware/efi/secureboot.c b/drivers/firmware/efi/secureboot.c
index 7292a3b832e3..be624a16996e 100644
--- a/drivers/firmware/efi/secureboot.c
+++ b/drivers/firmware/efi/secureboot.c
@@ -13,6 +13,8 @@
 #include <linux/kernel.h>
 #include <linux/printk.h>
 #include <linux/security.h>
+#include <linux/sysrq.h>
+#include <asm/efi.h>
 
 /*
  * Decide what to do when UEFI secure boot mode is enabled.
@@ -41,3 +43,38 @@ void __init efi_set_secure_boot(enum efi_secureboot_mode mode)
 		}
 	}
 }
+
+/*
+ * Allow secure boot to be lifted by pressing something like SysRq+x (and not
+ * by echoing the appropriate letter into the sysrq-trigger file).
+ */
+#ifdef CONFIG_EFI_ALLOW_SECURE_BOOT_EXIT
+
+static void sysrq_handle_secure_boot(int key)
+{
+	if (!efi_enabled(EFI_SECURE_BOOT))
+		return;
+
+	pr_info("Secure boot disabled\n");
+	lift_kernel_lockdown();
+}
+
+static struct sysrq_key_op secure_boot_sysrq_op = {
+	.handler	= sysrq_handle_secure_boot,
+	.help_msg	= "unSB(x)",
+	.action_msg	= "Disabling Secure Boot restrictions",
+	.enable_mask	= SYSRQ_DISABLE_USERSPACE,
+};
+
+static int __init efi_secure_boot_sysrq(void)
+{
+	if (efi_enabled(EFI_SECURE_BOOT)) {
+		secure_boot_sysrq_op.help_msg[5] = EFI_SECURE_BOOT_EXIT_KEY;
+		register_sysrq_key(EFI_SECURE_BOOT_EXIT_KEY, &secure_boot_sysrq_op);
+	}
+	return 0;
+}
+
+late_initcall(efi_secure_boot_sysrq);
+
+#endif /* CONFIG_EFI_ALLOW_SECURE_BOOT_EXIT */
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 022be0e22eba..4a054a564636 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -387,6 +387,7 @@ static int uinput_allocate_device(struct uinput_device *udev)
 	if (!udev->dev)
 		return -ENOMEM;
 
+	udev->dev->flags |= INPUTDEV_FLAGS_SYNTHETIC;
 	udev->dev->event = uinput_dev_event;
 	input_set_drvdata(udev->dev, udev);
 
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 3ffc1ce29023..8b766dbad6dd 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -481,6 +481,7 @@ static struct sysrq_key_op *sysrq_key_table[36] = {
 	/* x: May be registered on mips for TLB dump */
 	/* x: May be registered on ppc/powerpc for xmon */
 	/* x: May be registered on sparc64 for global PMU dump */
+	/* x: May be registered on x86_64 for disabling secure boot */
 	NULL,				/* x */
 	/* y: May be registered on sparc64 for global register dump */
 	NULL,				/* y */
@@ -524,7 +525,7 @@ static void __sysrq_put_key_op(int key, struct sysrq_key_op *op_p)
                 sysrq_key_table[i] = op_p;
 }
 
-void __handle_sysrq(int key, bool check_mask)
+void __handle_sysrq(int key, unsigned int from)
 {
 	struct sysrq_key_op *op_p;
 	int orig_log_level;
@@ -544,11 +545,15 @@ void __handle_sysrq(int key, bool check_mask)
 
         op_p = __sysrq_get_key_op(key);
         if (op_p) {
+		/* Ban synthetic events from some sysrq functionality */
+		if ((from == SYSRQ_FROM_PROC || from == SYSRQ_FROM_SYNTHETIC) &&
+		    op_p->enable_mask & SYSRQ_DISABLE_USERSPACE)
+			printk("This sysrq operation is disabled from userspace.\n");
 		/*
 		 * Should we check for enabled operations (/proc/sysrq-trigger
 		 * should not) and is the invoked operation enabled?
 		 */
-		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
+		if (from == SYSRQ_FROM_KERNEL || sysrq_on_mask(op_p->enable_mask)) {
 			pr_cont("%s\n", op_p->action_msg);
 			console_loglevel = orig_log_level;
 			op_p->handler(key);
@@ -580,7 +585,7 @@ void __handle_sysrq(int key, bool check_mask)
 void handle_sysrq(int key)
 {
 	if (sysrq_on())
-		__handle_sysrq(key, true);
+		__handle_sysrq(key, SYSRQ_FROM_KERNEL);
 }
 EXPORT_SYMBOL(handle_sysrq);
 
@@ -661,7 +666,7 @@ static void sysrq_do_reset(unsigned long _state)
 static void sysrq_handle_reset_request(struct sysrq_state *state)
 {
 	if (state->reset_requested)
-		__handle_sysrq(sysrq_xlate[KEY_B], false);
+		__handle_sysrq(sysrq_xlate[KEY_B], SYSRQ_FROM_KERNEL);
 
 	if (sysrq_reset_downtime_ms)
 		mod_timer(&state->keyreset_timer,
@@ -812,8 +817,10 @@ static bool sysrq_handle_keypress(struct sysrq_state *sysrq,
 
 	default:
 		if (sysrq->active && value && value != 2) {
+			int from = sysrq->handle.dev->flags & INPUTDEV_FLAGS_SYNTHETIC ?
+					SYSRQ_FROM_SYNTHETIC : 0;
 			sysrq->need_reinject = false;
-			__handle_sysrq(sysrq_xlate[code], true);
+			__handle_sysrq(sysrq_xlate[code], from);
 		}
 		break;
 	}
@@ -1097,7 +1104,7 @@ static ssize_t write_sysrq_trigger(struct file *file, const char __user *buf,
 
 		if (get_user(c, buf))
 			return -EFAULT;
-		__handle_sysrq(c, false);
+		__handle_sysrq(c, SYSRQ_FROM_PROC);
 	}
 
 	return count;
diff --git a/include/linux/input.h b/include/linux/input.h
index a65e3b24fb18..8b0357175049 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -42,6 +42,7 @@ struct input_value {
  * @phys: physical path to the device in the system hierarchy
  * @uniq: unique identification code for the device (if device has it)
  * @id: id of the device (struct input_id)
+ * @flags: input device flags (SYNTHETIC, etc.)
  * @propbit: bitmap of device properties and quirks
  * @evbit: bitmap of types of events supported by the device (EV_KEY,
  *	EV_REL, etc.)
@@ -124,6 +125,8 @@ struct input_dev {
 	const char *uniq;
 	struct input_id id;
 
+	unsigned int flags;
+
 	unsigned long propbit[BITS_TO_LONGS(INPUT_PROP_CNT)];
 
 	unsigned long evbit[BITS_TO_LONGS(EV_CNT)];
@@ -190,6 +193,8 @@ struct input_dev {
 };
 #define to_input_dev(d) container_of(d, struct input_dev, dev)
 
+#define	INPUTDEV_FLAGS_SYNTHETIC	0x000000001
+
 /*
  * Verify that we are in sync with input_device_id mod_devicetable.h #defines
  */
diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
index 387fa7d05c98..f7c52a9ea394 100644
--- a/include/linux/sysrq.h
+++ b/include/linux/sysrq.h
@@ -28,6 +28,8 @@
 #define SYSRQ_ENABLE_BOOT	0x0080
 #define SYSRQ_ENABLE_RTNICE	0x0100
 
+#define SYSRQ_DISABLE_USERSPACE	0x00010000
+
 struct sysrq_key_op {
 	void (*handler)(int);
 	char *help_msg;
@@ -42,8 +44,12 @@ struct sysrq_key_op {
  * are available -- else NULL's).
  */
 
+#define SYSRQ_FROM_KERNEL	0x0001
+#define SYSRQ_FROM_PROC		0x0002
+#define SYSRQ_FROM_SYNTHETIC	0x0004
+
 void handle_sysrq(int key);
-void __handle_sysrq(int key, bool check_mask);
+void __handle_sysrq(int key, unsigned int from);
 int register_sysrq_key(int key, struct sysrq_key_op *op);
 int unregister_sysrq_key(int key, struct sysrq_key_op *op);
 struct sysrq_key_op *__sysrq_get_key_op(int key);
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index c8146d53ca67..b480cadf9272 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1970,7 +1970,7 @@ static int kdb_sr(int argc, const char **argv)
 		return KDB_ARGCOUNT;
 
 	kdb_trap_printk++;
-	__handle_sysrq(*argv[1], check_mask);
+	__handle_sysrq(*argv[1], check_mask ? SYSRQ_FROM_KERNEL : 0);
 	kdb_trap_printk--;
 
 	return 0;

Re: [PATCH 5/5] Add a sysrq option to exit secure boot mode

[joeyli]

Hi,

On Wed, May 24, 2017 at 03:46:03PM +0100, David Howells wrote:
> From: Kyle McMartin <kyle@redhat.com>
> 
> Make sysrq+x exit secure boot mode on x86_64, thereby allowing the running
> kernel image to be modified.  This lifts the lockdown.
> 
> Signed-off-by: Kyle McMartin <kyle@redhat.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: x86@kernel.org
> ---
> 
>  arch/x86/include/asm/efi.h        |    2 ++
>  drivers/firmware/efi/Kconfig      |   10 ++++++++++
>  drivers/firmware/efi/secureboot.c |   37 +++++++++++++++++++++++++++++++++++++
>  drivers/input/misc/uinput.c       |    1 +
>  drivers/tty/sysrq.c               |   19 +++++++++++++------
>  include/linux/input.h             |    5 +++++
>  include/linux/sysrq.h             |    8 +++++++-
>  kernel/debug/kdb/kdb_main.c       |    2 +-
>  8 files changed, 76 insertions(+), 8 deletions(-)
> 
[...snip]  
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 3ffc1ce29023..8b766dbad6dd 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -481,6 +481,7 @@ static struct sysrq_key_op *sysrq_key_table[36] = {
>  	/* x: May be registered on mips for TLB dump */
>  	/* x: May be registered on ppc/powerpc for xmon */
>  	/* x: May be registered on sparc64 for global PMU dump */
> +	/* x: May be registered on x86_64 for disabling secure boot */
>  	NULL,				/* x */

I suggest that add this key to the "What are the 'command' keys?"
session in Documentation/admin-guide/sysrq.rst.

>  	/* y: May be registered on sparc64 for global register dump */
>  	NULL,				/* y */
> @@ -524,7 +525,7 @@ static void __sysrq_put_key_op(int key, struct sysrq_key_op *op_p)
>                  sysrq_key_table[i] = op_p;
>  }
>  
> -void __handle_sysrq(int key, bool check_mask)
> +void __handle_sysrq(int key, unsigned int from)
>  {
>  	struct sysrq_key_op *op_p;
>  	int orig_log_level;
> @@ -544,11 +545,15 @@ void __handle_sysrq(int key, bool check_mask)
>  
>          op_p = __sysrq_get_key_op(key);
>          if (op_p) {
> +		/* Ban synthetic events from some sysrq functionality */
> +		if ((from == SYSRQ_FROM_PROC || from == SYSRQ_FROM_SYNTHETIC) &&
> +		    op_p->enable_mask & SYSRQ_DISABLE_USERSPACE)
> +			printk("This sysrq operation is disabled from userspace.\n");
>  		/*
>  		 * Should we check for enabled operations (/proc/sysrq-trigger
>  		 * should not) and is the invoked operation enabled?
>  		 */
> -		if (!check_mask || sysrq_on_mask(op_p->enable_mask)) {
> +		if (from == SYSRQ_FROM_KERNEL || sysrq_on_mask(op_p->enable_mask)) {
>  			pr_cont("%s\n", op_p->action_msg);
>  			console_loglevel = orig_log_level;
>  			op_p->handler(key);

Looking at sysrq_on_mask():

static bool sysrq_on_mask(int mask)
{
	return sysrq_always_enabled ||
	       sysrq_enabled == 1 ||
	       (sysrq_enabled & mask); 
}

The SYSRQ_DISABLE_USERSPACE can be ignored by sysrq_always_enabled or
sysrq_enabled (the default value is CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE=0x1).
Kernel should checks the locked down flag here when secure boot ON. Will we
have another lock down patch against this? Or I missed something?

Thanks a lot!
Joey Lee

Re: [PATCH 5/5] Add a sysrq option to exit secure boot mode

[James Morris]

On Wed, 24 May 2017, David Howells wrote:

> From: Kyle McMartin <kyle-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Make sysrq+x exit secure boot mode on x86_64, thereby allowing the running
> kernel image to be modified.  This lifts the lockdown.
> 
> Signed-off-by: Kyle McMartin <kyle-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> cc: x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org


Reviewed-by: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>


-- 
James Morris
<jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[Ard Biesheuvel]

On 24 May 2017 at 14:45, David Howells <dhowells@redhat.com> wrote:
>
> Here's a set of patches to institute a "locked-down mode" in the kernel and
> to set that mode if the kernel is booted in secure-boot mode.  This can be
> enabled with CONFIG_LOCK_DOWN_KERNEL.  If a kernel is locked down, the
> lockdown can be lifted by typing SysRq+x on a keyboard attached to the
> machine if CONFIG_EFI_ALLOW_SECURE_BOOT_EXIT is enabled.  The exact key can
> be configured as 'x' is already taken on some arches.
>
> Inside the kernel, kernel_is_locked_down() is used to check if the kernel
> is in lockdown mode.  In lock-down mode, at least the following
> restrictions will need to be emplaced:
>
>  (1) No unsigned modules, kexec images or firmware.
>
>  (2) No direct read/write access of the kernel image.  (Shouldn't be able
>      to modify it and shouldn't be able to read out crypto data).
>
>  (3) No direct access to devices.  (DMA could be used to access/modify the
>      kernel image).
>
>  (4) No manual setting of device register addresses to cause a driver for
>      one device to mess around with another device, thereby permitting DMA.
>
>  (5) No storage of unencrypted kernel image to disk (no suspend-to-disk
>      without hardware support).
>
> I have patches pending that effect most of the above.  However, the
> firmware signature checking is being handled by someone else.  Further, it
> has come to light recently that debugfs needs attention, so that isn't done
> yet.
>
> Note that the secure boot mode entry doesn't currently work if the kernel
> is booted from current i386/x86_64 Grub as there's a bug in Grub whereby it
> doesn't initialise the boot_params correctly.  The incorrect initialisation
> causes sanitize_boot_params() to be triggered, thereby zapping the secure
> boot flag determined by the EFI boot wrapper.
>

Hello David,

By itself, this series looks in reasonable shape to me. But I do have
a few remaining concerns, apologies if it includes issues that I could
have brought up earlier.

- The series conflates 'UEFI secure boot support' with 'kernel lock
down support'. I think this has been brought up before, but I really
think we should have a cleaner separation between the feature (locking
down various bits of the kernel if lockdown is in effect) from the
policy 'enable lockdown if UEFI secure boot is enabled'. The latter
does not need to be configurable at all: on any UEFI system, we could
detect whether UEFI secure boot is in effect and report it. The only
tunable we need is in the lockdown context, whether lockdown needs to
take effect automatically when UEFI secure boot is detected (but there
could be other ways to enable lockdown, including a kernel cmdline
param or a sysfs node). Similarly, whether lockdown can be lifted or
not has *nothing* to do with whether it was enabled due to UEFI secure
boot, so I don't see the point of having
CONFIG_EFI_ALLOW_SECURE_BOOT_EXIT at all.

- The current series enables lockdown, but does not lock anything
down. Even if the code is in good shape otherwise, I am reluctant to
ack and/or merge anything right now, given that it provides a false
sense of security. This also ties in to my more general concerns with
this code (and I am aware I never replied to your email explaining it,
my apologies [again]): without any sense of how large the attack
surface is now, and how much we reduced it by implementing the items
on your list above, we should really not be making claims of security,
given that we really have no idea how much more secure we are. That
said, I do subscribe to the effort, in the sense that moving towards
the goal is strictly better than moving away from it, or not at all.

- Patch 5/5 breaks the build on non-x86. Please build test EFI-related
patches on arm64 and/or ARM before submitting patches. And if
possible, could we find a magic SysRq key that works on all
architectures? I know we discussed this at some point (I think?) but I
don't remember the conclusion.

Regards,
Ard.


> ---
> David Howells (3):
>       efi: Move the x86 secure boot switch to generic code
>       Add the ability to lock down access to the running kernel image
>       efi: Lock down the kernel if booted in secure boot mode
>
> Josh Boyer (1):
>       efi: Add EFI_SECURE_BOOT bit
>
> Kyle McMartin (1):
>       Add a sysrq option to exit secure boot mode
>
>
>  arch/x86/include/asm/efi.h        |    2 +
>  arch/x86/kernel/setup.c           |   14 ------
>  drivers/firmware/efi/Kconfig      |   34 ++++++++++++++++
>  drivers/firmware/efi/Makefile     |    1
>  drivers/firmware/efi/secureboot.c |   80 +++++++++++++++++++++++++++++++++++++
>  drivers/input/misc/uinput.c       |    1
>  drivers/tty/sysrq.c               |   19 ++++++---
>  include/linux/efi.h               |    7 +++
>  include/linux/input.h             |    5 ++
>  include/linux/kernel.h            |    9 ++++
>  include/linux/security.h          |   11 +++++
>  include/linux/sysrq.h             |    8 +++-
>  kernel/debug/kdb/kdb_main.c       |    2 -
>  security/Kconfig                  |   15 +++++++
>  security/Makefile                 |    3 +
>  security/lock_down.c              |   46 +++++++++++++++++++++
>  16 files changed, 236 insertions(+), 21 deletions(-)
>  create mode 100644 drivers/firmware/efi/secureboot.c
>  create mode 100644 security/lock_down.c
>

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[David Howells]

Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> - The series conflates 'UEFI secure boot support' with 'kernel lock
> down support'. I think this has been brought up before, but I really
> think we should have a cleaner separation between the feature (locking
> down various bits of the kernel if lockdown is in effect) from the
> policy 'enable lockdown if UEFI secure boot is enabled'.

I'm not sure what you're actually asking for.  Are you wanting me to push the
lockdown patches upstream separately from the UEFI patches that trigger the
lockdown?  Or do you mean something else?

> The only tunable we need is in the lockdown context, whether lockdown needs
> to take effect automatically when UEFI secure boot is detected

So you don't want this change:

	config EFI_SECURE_BOOT
		bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
		default n
	+	select LOCK_DOWN_KERNEL

but would rather have a separate option asking whether lockdown should be
triggered if detect UEFI secure boot mode?

> (but there could be other ways to enable lockdown, including a kernel
> cmdline param or a sysfs node).

A sysfs node is pretty pointless.  A number of things that are locked down
need to be locked down during boot.  I could, however, provide a command-line
option to engage it or a kernel config option to unconditionally enable it.

> - The current series enables lockdown, but does not lock anything
> down.

Yes.  As I pointed out, and as I'm sure you know, I have a slew of other
patches that actually *do* lock things down.  I extracted these patches to try
and get some feedback on this bit without spamming various mailing lists with
all the other patches each time.

> Even if the code is in good shape otherwise, I am reluctant to
> ack and/or merge anything right now, given that it provides a false
> sense of security.

You're a member of the "make it provably 100% secure or nothing" camp?

> This also ties in to my more general concerns with this code (and I am aware
> I never replied to your email explaining it, my apologies [again]): without
> any sense of how large the attack surface is now,

I suspect no one knows.  I've been trying to lock down possibilities people
have pointed me at, even if some of them are very theoretical, and a lot of
the patches I've gathered together come from other people, but I don't know
the hardware that a lot of this is dealing with, so I can't answer this
question.

> and how much we reduced it by implementing the items on your list above, we
> should really not be making claims of security, given that we really have no
> idea how much more secure we are. That said, I do subscribe to the effort,
> in the sense that moving towards the goal is strictly better than moving
> away from it, or not at all.

Can we at least decide whether or not we want to put a locked-down mode into
the upstream kernel?  If not, I can drop this effort.

David

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[Ard Biesheuvel]

On 31 May 2017 at 09:23, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> - The series conflates 'UEFI secure boot support' with 'kernel lock
>> down support'. I think this has been brought up before, but I really
>> think we should have a cleaner separation between the feature (locking
>> down various bits of the kernel if lockdown is in effect) from the
>> policy 'enable lockdown if UEFI secure boot is enabled'.
>
> I'm not sure what you're actually asking for.  Are you wanting me to push the
> lockdown patches upstream separately from the UEFI patches that trigger the
> lockdown?  Or do you mean something else?
>

No, I am fine with keeping this as a single series. I don't want
anything under drivers/efi to imply policy regarding lockdown. Kernel
lockdown should be a feature that lives somewhere else, and which
contains a CONFIG_ option that implies 'lockdown is enabled by default
when UEFI secure boot is detected.' The code that gets added to
drivers/efi should only concern itself with establishing whether
secure boot is in effect or not (and can hence be enabled
unconditionally)

>> The only tunable we need is in the lockdown context, whether lockdown needs
>> to take effect automatically when UEFI secure boot is detected
>
> So you don't want this change:
>
>         config EFI_SECURE_BOOT
>                 bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
>                 default n
>         +       select LOCK_DOWN_KERNEL
>
> but would rather have a separate option asking whether lockdown should be
> triggered if detect UEFI secure boot mode?
>

Yes. As I said, supporting UEFI secure boot (whatever that could mean)
and implementing a lockdown policy when it is enabled are two entirely
different things.

>> (but there could be other ways to enable lockdown, including a kernel
>> cmdline param or a sysfs node).
>
> A sysfs node is pretty pointless.  A number of things that are locked down
> need to be locked down during boot.

OK.

> I could, however, provide a command-line
> option to engage it or a kernel config option to unconditionally enable it.
>

I *think* that could be useful, although it was meant as an
illustration rather than actual suggestion.

>> - The current series enables lockdown, but does not lock anything
>> down.
>
> Yes.  As I pointed out, and as I'm sure you know, I have a slew of other
> patches that actually *do* lock things down.  I extracted these patches to try
> and get some feedback on this bit without spamming various mailing lists with
> all the other patches each time.
>
>> Even if the code is in good shape otherwise, I am reluctant to
>> ack and/or merge anything right now, given that it provides a false
>> sense of security.
>
> You're a member of the "make it provably 100% secure or nothing" camp?
>

No, not at all. But patch 4/5 contains this line

pr_info("Secure boot enabled and kernel locked down\n");

which I don't like at all, given that nothing is actually locked down.
I have been working in this area long enough to know that people will
interpret 'lockdown' or 'secure' to mean anything they like if it is
left unqualified.

So what I would prefer is to separate this from the EFI code, and
perhaps print something like

lockdown: Kernel lockdown policy in effect due to xxx

and print a subsequent line for every lockdown feature that is enabled, e.g.,

lockdown: disabling MSRs
lockdown: disabling hibernate support

etc etc

These are just examples, of course, but it manages the expectations,
and as a bonus, it makes differences between architectures much more
visible.

>> This also ties in to my more general concerns with this code (and I am aware
>> I never replied to your email explaining it, my apologies [again]): without
>> any sense of how large the attack surface is now,
>
> I suspect no one knows.  I've been trying to lock down possibilities people
> have pointed me at, even if some of them are very theoretical, and a lot of
> the patches I've gathered together come from other people, but I don't know
> the hardware that a lot of this is dealing with, so I can't answer this
> question.
>

OK, and that is fine. I would just like this modest stance to be
reflected in the code and especially the log messages.

>> and how much we reduced it by implementing the items on your list above, we
>> should really not be making claims of security, given that we really have no
>> idea how much more secure we are. That said, I do subscribe to the effort,
>> in the sense that moving towards the goal is strictly better than moving
>> away from it, or not at all.
>
> Can we at least decide whether or not we want to put a locked-down mode into
> the upstream kernel?  If not, I can drop this effort.

No, I think this is a very important feature. But it needs to be more
transparent to avoid a false sense of security.

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[David Howells]

Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> No, I am fine with keeping this as a single series. I don't want
> anything under drivers/efi to imply policy regarding lockdown. Kernel
> lockdown should be a feature that lives somewhere else, and which
> contains a CONFIG_ option that implies 'lockdown is enabled by default
> when UEFI secure boot is detected.' The code that gets added to
> drivers/efi should only concern itself with establishing whether
> secure boot is in effect or not (and can hence be enabled
> unconditionally)
> ...
> So what I would prefer is to separate this from the EFI code,

In that case I don't know where to connect the UEFI secure boot with the
lockdown code.

I was under the impression that you wanted the switch-statement that I had in
x86 setup.c moved to the efi code (as I've done in patch 1).  Was I wrong in
that assessment and that you actually wanted it, say, in security?

I don't think that the non-EFI core code should know about UEFI secure boot
mode.  Either the arch needs to implement the connection or the EFI code needs
to implement it.  In the former is preferred, I should drop patch 1.

> ... and perhaps print something like
> 
> lockdown: Kernel lockdown policy in effect due to xxx

I'm okay with printing that instead.

> and print a subsequent line for every lockdown feature that is enabled, e.g.,
> 
> lockdown: disabling MSRs
> lockdown: disabling hibernate support

That could add a lot of lines to the boot output:-/

David

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[Ard Biesheuvel]

On 31 May 2017 at 13:33, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> No, I am fine with keeping this as a single series. I don't want
>> anything under drivers/efi to imply policy regarding lockdown. Kernel
>> lockdown should be a feature that lives somewhere else, and which
>> contains a CONFIG_ option that implies 'lockdown is enabled by default
>> when UEFI secure boot is detected.' The code that gets added to
>> drivers/efi should only concern itself with establishing whether
>> secure boot is in effect or not (and can hence be enabled
>> unconditionally)
>> ...
>> So what I would prefer is to separate this from the EFI code,
>
> In that case I don't know where to connect the UEFI secure boot with the
> lockdown code.
>
> I was under the impression that you wanted the switch-statement that I had in
> x86 setup.c moved to the efi code (as I've done in patch 1).  Was I wrong in
> that assessment and that you actually wanted it, say, in security?
>

No, that patch, and the patch that sets the EFI_SECURE_BOOT flag are
perfectly fine. I just think it should be the lockdown code that
contains the efi_enabled(EFI_SECURE_BOOT) check. Note that linux/efi.h
does the right thing in case CONFIG_EFI is not defined.

> I don't think that the non-EFI core code should know about UEFI secure boot
> mode.  Either the arch needs to implement the connection or the EFI code needs
> to implement it.  In the former is preferred, I should drop patch 1.
>
>> ... and perhaps print something like
>>
>> lockdown: Kernel lockdown policy in effect due to xxx
>
> I'm okay with printing that instead.
>
>> and print a subsequent line for every lockdown feature that is enabled, e.g.,
>>
>> lockdown: disabling MSRs
>> lockdown: disabling hibernate support
>
> That could add a lot of lines to the boot output:-/
>

Why is that a bad thing?

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[David Howells]

Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> and print a subsequent line for every lockdown feature that is enabled, e.g.,
> 
> lockdown: disabling MSRs
> lockdown: disabling hibernate support

There's another problem with this idea: the lockdown facility is passive - it
doesn't go looking for things to lock down; rather, things that can be locked
down inquire as to whether lockdown is in effect at the point someone tries to
use them.

Now, I could reserve a variable for each thing we lock down to make sure that
we don't emit the message more than once, but I'm loathe to waste memory this
way.

I can't so easily switch the facility to being active either, since a lot of
the lockdownables are in modules.

David

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[Ard Biesheuvel]

(+ Kees)

On 6 June 2017 at 09:34, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> and print a subsequent line for every lockdown feature that is enabled, e.g.,
>>
>> lockdown: disabling MSRs
>> lockdown: disabling hibernate support
>
> There's another problem with this idea: the lockdown facility is passive - it
> doesn't go looking for things to lock down; rather, things that can be locked
> down inquire as to whether lockdown is in effect at the point someone tries to
> use them.
>
> Now, I could reserve a variable for each thing we lock down to make sure that
> we don't emit the message more than once, but I'm loathe to waste memory this
> way.
>
> I can't so easily switch the facility to being active either, since a lot of
> the lockdownables are in modules.
>

Let me reiterate my concern for Kees's sake: without a threat model
nor any notion of how much kernel lockdown reduces the attack surface,
we end up with a 'doing something is better than nothing' approach.
Fine. But now you are telling me that there is no way we can provide
information to the user what that 'something' amounts to for a
particular kernel build for a particular architecture. Do you intend
to add lockdown features going forward after enabling the initial set?
Would any of these lockdown features ever be Kconfigurable? Do you
expect all lockdown features to be applicable to all architectures? So
how do I know what lockdown actually means for my system?

Anyway, I think I have made my concerns clear, and the EFI bits look
fine to me as long as the policy lives elsewhere.

Apologies for letting this drag on a bit. I do agree in principle, but
I'd like to get the details right as well.

Thanks,
Ard.

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

[Kees Cook]

On Fri, Jun 9, 2017 at 10:33 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> (+ Kees)
>
> On 6 June 2017 at 09:34, David Howells <dhowells@redhat.com> wrote:
>> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>> and print a subsequent line for every lockdown feature that is enabled, e.g.,
>>>
>>> lockdown: disabling MSRs
>>> lockdown: disabling hibernate support

If lockdown introspection is desired, perhaps just add a /proc file
that lists them all? And if memory usage is a concern, it could be
behind another config, like CONFIG_LOCK_DOWN_KERNEL_PROC ?

>> There's another problem with this idea: the lockdown facility is passive - it
>> doesn't go looking for things to lock down; rather, things that can be locked
>> down inquire as to whether lockdown is in effect at the point someone tries to
>> use them.
>>
>> Now, I could reserve a variable for each thing we lock down to make sure that
>> we don't emit the message more than once, but I'm loathe to waste memory this
>> way.
>>
>> I can't so easily switch the facility to being active either, since a lot of
>> the lockdownables are in modules.

I personally don't think the introspection is needed, but I see Ard's point.

> Let me reiterate my concern for Kees's sake: without a threat model
> nor any notion of how much kernel lockdown reduces the attack surface,
> we end up with a 'doing something is better than nothing' approach.
> Fine. But now you are telling me that there is no way we can provide
> information to the user what that 'something' amounts to for a
> particular kernel build for a particular architecture. Do you intend
> to add lockdown features going forward after enabling the initial set?
> Would any of these lockdown features ever be Kconfigurable? Do you
> expect all lockdown features to be applicable to all architectures? So
> how do I know what lockdown actually means for my system?

I'd like the kernel to grow a bright line between userspace and
physical memory, so I'm a fan of this series. It does look rather
ad-hoc currently, but I think that's the reality the kernel faces.
Right now, we're in a position where the kernel APIs don't provide a
mechanism to distinguish between userspace users and kernel users (in
that anything can just expose a kernel API to userspace: e.g. all the
drivers over the years that trip over STRICT_DEVMEM and then create
their own /dev/mem without restrictions).

So, as we stand, we need to start by adding lockdown checks everywhere
we find exposures, and I'd expect this list to grow over time (which
would support the desirability of introspection, but I kind of see
introspection as overkill). It'd be nice if we could identify classes
of things that have to keep getting lockdown logic added to so that we
could collect them into a single place (e.g. module parameters that
define a phyiscal memory area could use a helper to do it instead of
being yet another unsigned long, and the helper would do the lockdown
check, etc). But that work will take time.

> Anyway, I think I have made my concerns clear, and the EFI bits look
> fine to me as long as the policy lives elsewhere.

Yeah, splitting policy makes sense to me: Chrome OS would use lockdown
as well, but it doesn't use EFI at all.

> Apologies for letting this drag on a bit. I do agree in principle, but
> I'd like to get the details right as well.

(And please CC me on future versions.)

-Kees

-- 
Kees Cook
Pixel Security