[PATCH 00/27] security, efi: Add kernel lockdown

[PATCH 00/27] security, efi: Add kernel lockdown

[David Howells]

Here's a set of patches to institute a "locked-down mode" in the kernel and
to trigger that mode if the kernel is booted in secure-boot mode or through
the command line.

Enabling CONFIG_LOCK_DOWN_KERNEL makes lockdown mode available.

Enabling CONFIG_ALLOW_LOCKDOWN_LIFT_BY_SYSRQ will allow a SysRq combination
to lift the lockdown.  On x86 this is SysRq+x.  The keys must be pressed on
an attached keyboard.

Enabling CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT will cause EFI secure boot to
trigger kernel lockdown.

Inside the kernel, kernel_is_locked_down() is used to check if the kernel
is in lockdown mode.

Note that the secure boot mode entry doesn't work if the kernel is booted
from older versions of 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.

A manual page, kernel_lockdown.7, is proposed, to which people will be
directed by messages in dmesg.  This lists the features that are restricted
amongst other things.

I'm aware there may be things that aren't yet handled, but we can add those
later.

====================
PROPOSED MANUAL PAGE
====================

.\"
.\" Copyright (C) 2017 Red Hat, Inc. All Rights Reserved.
.\" Written by David Howells (dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org)
.\"
.\" % % %LICENSE_START(GPLv2+_SW_ONEPARA)
.\" This program is free software; you can redistribute it and/or
.\" modify it under the terms of the GNU General Public License
.\" as published by the Free Software Foundation; either version
.\" 2 of the License, or (at your option) any later version.
.\" % % %LICENSE_END
.\"
.TH "KERNEL LOCKDOWN" 7 2017-10-05 Linux "Linux Programmer's Manual"
.SH NAME
Kernel Lockdown \- Kernel image access prevention feature
.SH DESCRIPTION
The Kernel Lockdown feature is designed to prevent both direct and indirect
access to a running kernel image, attempting to protect against unauthorised
modification of the kernel image and to prevent access to security and
cryptographic data located in kernel memory, whilst still permitting driver
modules to be loaded.
.P
Lockdown is typically enabled during boot and may be terminated, if configured,
by typing a special key combination on a directly attached physical keyboard.
.P
If a prohibited or restricted feature is accessed or used, the kernel will emit
a message that looks like:
.P
.RS
	Lockdown: X is restricted, see man kernel_lockdown.7
.RE
.P
where X indicates what is restricted.
.P
On an EFI-enabled x86 or arm64 machine, lockdown will be automatically enabled
if the system boots in EFI Secure Boot mode.
.P
If the kernel is appropriately configured, lockdown may be lifted by typing the
appropriate sequence on a directly attached physical keyboard.  For x86
machines, this is
.IR SysRq+x .
.\"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""
.SH COVERAGE
When lockdown is in effect, a number of things are disabled or restricted in
use.  This includes special device files and kernel services that allow direct
access of the kernel image:
.P
.RS
/dev/mem
.br
/dev/kmem
.br
/dev/kcore
.br
/dev/ioports
.br
BPF memory access functions
.RE
.P
and the ability to directly configure and control devices, so as to prevent the
use of a device to access or modify a kernel image:
.P
.RS
The use of module parameters that directly specify hardware parameters to
drivers through the kernel command line or when loading a module.
.P
The use of direct PCI BAR access.
.P
The use of the ioperm and iopl instructions on x86.
.P
The use of the KD*IO console ioctls.
.P
The use of the TIOCSSERIAL serial ioctl.
.P
The alteration of MSR registers on x86.
.P
The replacement of the PCMCIA CIS.
.P
The overriding of ACPI tables.
.P
The use of ACPI error injection.
.P
The specification of the ACPI RDSP address.
.P
The use of ACPI custom methods.
.RE
.P
The following facilities are restricted:
.P
.RS
Only validly signed modules may be loaded.
.P
Only validly signed binaries may be kexec'd.
.P
Only validly signed device firmware may be loaded.
.P
Unencrypted hibernation/suspend to swap are disallowed as the kernel image is
saved to a medium that can then be accessed.
.P
Use of debugfs is not permitted as this allows a whole range of actions
including direct configuration of, access to and driving of hardware.
.RE
.\"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""
.SH SEE ALSO
.ad l
.nh


The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=efi-lock-down

Tagged thusly:

	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
	lockdown-20171019

David
---
Chun-Yi Lee (2):
      kexec_file: Disable at runtime if securelevel has been set
      bpf: Restrict kernel image access functions when the kernel is locked down

Dave Young (1):
      Copy secure_boot flag in boot params across kexec reboot

David Howells (11):
      Add the ability to lock down access to the running kernel image
      Enforce module signatures if the kernel is locked down
      scsi: Lock down the eata driver
      Prohibit PCMCIA CIS storage when the kernel is locked down
      Lock down TIOCSSERIAL
      Lock down module params that specify hardware parameters (eg. ioport)
      x86/mmiotrace: Lock down the testmmiotrace module
      debugfs: Disallow use of debugfs files when the kernel is locked down
      Lock down /proc/kcore
      efi: Add an EFI_SECURE_BOOT flag to indicate secure boot mode
      efi: Lock down the kernel if booted in secure boot mode

Josh Boyer (2):
      hibernate: Disable when the kernel is locked down
      acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down

Kyle McMartin (1):
      Add a SysRq option to lift kernel lockdown

Linn Crosetto (2):
      acpi: Disable ACPI table override if the kernel is locked down
      acpi: Disable APEI error injection if the kernel is locked down

Matthew Garrett (8):
      Restrict /dev/mem and /dev/kmem when the kernel is locked down
      kexec: Disable at runtime if the kernel is locked down
      uswsusp: Disable when the kernel is locked down
      PCI: Lock down BAR access when the kernel is locked down
      x86: Lock down IO port access when the kernel is locked down
      x86/msr: Restrict MSR access when the kernel is locked down
      asus-wmi: Restrict debugfs interface when the kernel is locked down
      ACPI: Limit access to custom_method when the kernel is locked down


 arch/x86/include/asm/setup.h      |    2 +
 arch/x86/kernel/ioport.c          |    6 +-
 arch/x86/kernel/kexec-bzimage64.c |    1 
 arch/x86/kernel/msr.c             |    7 ++
 arch/x86/kernel/setup.c           |   18 +-----
 arch/x86/mm/testmmiotrace.c       |    3 +
 drivers/acpi/apei/einj.c          |    3 +
 drivers/acpi/custom_method.c      |    3 +
 drivers/acpi/osl.c                |    2 -
 drivers/acpi/tables.c             |    5 ++
 drivers/char/mem.c                |    8 +++
 drivers/firmware/efi/Makefile     |    1 
 drivers/firmware/efi/secureboot.c |   37 +++++++++++++
 drivers/input/misc/uinput.c       |    1 
 drivers/pci/pci-sysfs.c           |    9 +++
 drivers/pci/proc.c                |    9 +++
 drivers/pci/syscall.c             |    3 +
 drivers/pcmcia/cistpl.c           |    3 +
 drivers/platform/x86/asus-wmi.c   |    9 +++
 drivers/scsi/eata.c               |    5 +-
 drivers/tty/serial/serial_core.c  |    6 ++
 drivers/tty/sysrq.c               |   19 ++++--
 fs/debugfs/file.c                 |    6 ++
 fs/proc/kcore.c                   |    2 +
 include/linux/efi.h               |   16 +++--
 include/linux/input.h             |    5 ++
 include/linux/kernel.h            |   17 ++++++
 include/linux/security.h          |    8 +++
 include/linux/sysrq.h             |    8 ++-
 kernel/debug/kdb/kdb_main.c       |    2 -
 kernel/kexec.c                    |    7 ++
 kernel/kexec_file.c               |    7 ++
 kernel/module.c                   |    3 +
 kernel/params.c                   |   26 +++++++--
 kernel/power/hibernate.c          |    2 -
 kernel/power/user.c               |    3 +
 kernel/trace/bpf_trace.c          |   11 ++++
 security/Kconfig                  |   37 +++++++++++++
 security/Makefile                 |    3 +
 security/lock_down.c              |  109 +++++++++++++++++++++++++++++++++++++
 40 files changed, 391 insertions(+), 41 deletions(-)
 create mode 100644 drivers/firmware/efi/secureboot.c
 create mode 100644 security/lock_down.c

[PATCH 01/27] 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>
---

 include/linux/kernel.h   |   17 +++++++++++++
 include/linux/security.h |    8 ++++++
 security/Kconfig         |    8 ++++++
 security/Makefile        |    3 ++
 security/lock_down.c     |   60 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 96 insertions(+)
 create mode 100644 security/lock_down.c

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0ad4c3044cf9..362da2e4bf53 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -287,6 +287,23 @@ static inline void refcount_error_report(struct pt_regs *regs, const char *err)
 { }
 #endif
 
+#ifdef CONFIG_LOCK_DOWN_KERNEL
+extern bool __kernel_is_locked_down(const char *what, bool first);
+#else
+static inline bool __kernel_is_locked_down(const char *what, bool first)
+{
+	return false;
+}
+#endif
+
+#define kernel_is_locked_down(what)					\
+	({								\
+		static bool message_given;				\
+		bool locked_down = __kernel_is_locked_down(what, !message_given); \
+		message_given = true;					\
+		locked_down;						\
+	})
+
 /* 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 ce6265960d6c..f9a894b42d4c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1753,5 +1753,13 @@ static inline void free_secdata(void *secdata)
 { }
 #endif /* CONFIG_SECURITY */
 
+#ifdef CONFIG_LOCK_DOWN_KERNEL
+extern void __init init_lockdown(void);
+#else
+static inline void __init init_lockdown(void);
+{
+}
+#endif
+
 #endif /* ! __LINUX_SECURITY_H */
 
diff --git a/security/Kconfig b/security/Kconfig
index e8e449444e65..8e01fd59ae7e 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -205,6 +205,14 @@ 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).
+
 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..d8595c0e6673
--- /dev/null
+++ b/security/lock_down.c
@@ -0,0 +1,60 @@
+/* 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>
+
+static __ro_after_init bool kernel_locked_down;
+
+/*
+ * Put the kernel into lock-down mode.
+ */
+static void __init lock_kernel_down(const char *where)
+{
+	if (!kernel_locked_down) {
+		kernel_locked_down = true;
+		pr_notice("Kernel is locked down from %s; see man kernel_lockdown.7\n",
+			  where);
+	}
+}
+
+static int __init lockdown_param(char *ignored)
+{
+	lock_kernel_down("command line");
+	return 0;
+}
+
+early_param("lockdown", lockdown_param);
+
+/*
+ * Lock the kernel down from very early in the arch setup.  This must happen
+ * prior to things like ACPI being initialised.
+ */
+void __init init_lockdown(void)
+{
+#ifdef CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT
+	if (efi_enabled(EFI_SECURE_BOOT))
+		lock_kernel_down("EFI secure boot");
+#endif
+}
+
+/**
+ * kernel_is_locked_down - Find out if the kernel is locked down
+ * @what: Tag to use in notice generated if lockdown is in effect
+ */
+bool __kernel_is_locked_down(const char *what, bool first)
+{
+	if (what && first && kernel_locked_down)
+		pr_notice("Lockdown: %s is restricted; see man kernel_lockdown.7\n",
+			  what);
+	return kernel_locked_down;
+}
+EXPORT_SYMBOL(__kernel_is_locked_down);

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

[James Morris]

On Thu, 19 Oct 2017, 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@redhat.com>


Acked-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<james.l.morris@oracle.com>

[PATCH 02/27] Add a SysRq option to lift kernel lockdown

[David Howells]

From: Kyle McMartin <kyle@redhat.com>

Make an option to provide a sysrq key that will lift the kernel lockdown,
thereby allowing the running kernel image to be accessed and modified.

On x86_64 this is triggered with SysRq+x, but this key may not be available
on all arches, so it is set by setting LOCKDOWN_LIFT_KEY in asm/setup.h.

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

 arch/x86/include/asm/setup.h |    2 ++
 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 +-
 security/Kconfig             |   15 +++++++++++++
 security/lock_down.c         |   48 ++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 92 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
index a65cf544686a..863f77582c09 100644
--- a/arch/x86/include/asm/setup.h
+++ b/arch/x86/include/asm/setup.h
@@ -8,6 +8,8 @@
 #include <linux/linkage.h>
 #include <asm/page_types.h>
 
+#define LOCKDOWN_LIFT_KEY 'x'
+
 #ifdef __i386__
 
 #include <linux/pfn.h>
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 443151de90c6..45a1f5460805 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -408,6 +408,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 fb5e23c7ed98..9d2b45a21ade 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;
diff --git a/security/Kconfig b/security/Kconfig
index 8e01fd59ae7e..4be6be71e075 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -213,6 +213,21 @@ config LOCK_DOWN_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).
+
+config ALLOW_LOCKDOWN_LIFT_BY_SYSRQ
+	bool "Allow the kernel lockdown to be lifted by SysRq"
+	depends on MAGIC_SYSRQ
+	help
+	  Allow the lockdown on a kernel to be lifted, by pressing a SysRq key
+	  combination on a wired keyboard.
+
+
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
diff --git a/security/lock_down.c b/security/lock_down.c
index d8595c0e6673..f71118c340d2 100644
--- a/security/lock_down.c
+++ b/security/lock_down.c
@@ -11,8 +11,13 @@
 
 #include <linux/security.h>
 #include <linux/export.h>
+#include <linux/sysrq.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.
@@ -58,3 +63,46 @@ bool __kernel_is_locked_down(const char *what, bool first)
 	return kernel_locked_down;
 }
 EXPORT_SYMBOL(__kernel_is_locked_down);
+
+/*
+ * Take the kernel out of lockdown mode.
+ */
+#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
+static void lift_kernel_lockdown(void)
+{
+	pr_notice("Lifting lockdown\n");
+	kernel_locked_down = false;
+}
+#endif
+
+/*
+ * Allow lockdown to be lifted by pressing something like SysRq+x (and not by
+ * echoing the appropriate letter into the sysrq-trigger file).
+ */
+#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT_BY_KEY
+
+static void sysrq_handle_lockdown_lift(int key)
+{
+	if (kernel_locked_down)
+		lift_kernel_lockdown();
+}
+
+static struct sysrq_key_op lockdown_lift_sysrq_op = {
+	.handler	= sysrq_handle_lockdown_lift,
+	.help_msg	= "unSB(x)",
+	.action_msg	= "Disabling Secure Boot restrictions",
+	.enable_mask	= SYSRQ_DISABLE_USERSPACE,
+};
+
+static int __init lockdown_lift_sysrq(void)
+{
+	if (kernel_locked_down) {
+		lockdown_lift_sysrq_op.help_msg[5] = LOCKDOWN_LIFT_KEY;
+		register_sysrq_key(LOCKDOWN_LIFT_KEY, &lockdown_lift_sysrq_op);
+	}
+	return 0;
+}
+
+late_initcall(lockdown_lift_sysrq);
+
+#endif /* CONFIG_ALLOW_LOCKDOWN_LIFT_BY_KEY */

Re: [PATCH 02/27] Add a SysRq option to lift kernel lockdown

[Randy Dunlap]

On 10/19/17 07:50, David Howells wrote:
> From: Kyle McMartin <kyle-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Make an option to provide a sysrq key that will lift the kernel lockdown,
> thereby allowing the running kernel image to be accessed and modified.
> 
> On x86_64 this is triggered with SysRq+x, but this key may not be available
> on all arches, so it is set by setting LOCKDOWN_LIFT_KEY in asm/setup.h.
> 
> 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
> ---
> 
>  arch/x86/include/asm/setup.h |    2 ++
>  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 +-
>  security/Kconfig             |   15 +++++++++++++
>  security/lock_down.c         |   48 ++++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 92 insertions(+), 8 deletions(-)

> diff --git a/security/Kconfig b/security/Kconfig
> index 8e01fd59ae7e..4be6be71e075 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -213,6 +213,21 @@ config LOCK_DOWN_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

how about:                                                                on

> +	  x86).
> +
> +config ALLOW_LOCKDOWN_LIFT_BY_SYSRQ
> +	bool "Allow the kernel lockdown to be lifted by SysRq"
> +	depends on MAGIC_SYSRQ
> +	help
> +	  Allow the lockdown on a kernel to be lifted, by pressing a SysRq key
> +	  combination on a wired keyboard.
> +
> +
>  source security/selinux/Kconfig
>  source security/smack/Kconfig
>  source security/tomoyo/Kconfig
> diff --git a/security/lock_down.c b/security/lock_down.c
> index d8595c0e6673..f71118c340d2 100644
> --- a/security/lock_down.c
> +++ b/security/lock_down.c


> +
> +/*
> + * Allow lockdown to be lifted by pressing something like SysRq+x (and not by
> + * echoing the appropriate letter into the sysrq-trigger file).
> + */
> +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT_BY_KEY

is that the same as: CONFIG_ALLOW_LOCKDOWN_LIFT_BY_SYSRQ ?
tested?

> +
> +static void sysrq_handle_lockdown_lift(int key)
> +{
> +	if (kernel_locked_down)
> +		lift_kernel_lockdown();
> +}
> +
> +static struct sysrq_key_op lockdown_lift_sysrq_op = {
> +	.handler	= sysrq_handle_lockdown_lift,
> +	.help_msg	= "unSB(x)",
> +	.action_msg	= "Disabling Secure Boot restrictions",
> +	.enable_mask	= SYSRQ_DISABLE_USERSPACE,
> +};
> +
> +static int __init lockdown_lift_sysrq(void)
> +{
> +	if (kernel_locked_down) {
> +		lockdown_lift_sysrq_op.help_msg[5] = LOCKDOWN_LIFT_KEY;
> +		register_sysrq_key(LOCKDOWN_LIFT_KEY, &lockdown_lift_sysrq_op);
> +	}
> +	return 0;
> +}
> +
> +late_initcall(lockdown_lift_sysrq);
> +
> +#endif /* CONFIG_ALLOW_LOCKDOWN_LIFT_BY_KEY */

                                        BY_SYSRQ


-- 
~Randy

Re: [PATCH 02/27] Add a SysRq option to lift kernel lockdown

[David Howells]

Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> > +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
> 
> how about:                                                                on
> 
> > +	  x86).

I'll just get rid of this config option, I think - it doesn't make anything
available outside of lock_down.c.

> > +#ifdef CONFIG_ALLOW_LOCKDOWN_LIFT_BY_KEY
> 
> is that the same as: CONFIG_ALLOW_LOCKDOWN_LIFT_BY_SYSRQ ?
> tested?

My test machine doesn't have a physical keyboard attached, but you're right.

David

Re: [PATCH 02/27] Add a SysRq option to lift kernel lockdown

[Thiago Jung Bauermann]

Hello David,

David Howells <dhowells@redhat.com> writes:
> +static struct sysrq_key_op lockdown_lift_sysrq_op = {
> +	.handler	= sysrq_handle_lockdown_lift,
> +	.help_msg	= "unSB(x)",
> +	.action_msg	= "Disabling Secure Boot restrictions",
> +	.enable_mask	= SYSRQ_DISABLE_USERSPACE,
> +};
> +
> +static int __init lockdown_lift_sysrq(void)
> +{
> +	if (kernel_locked_down) {
> +		lockdown_lift_sysrq_op.help_msg[5] = LOCKDOWN_LIFT_KEY;
> +		register_sysrq_key(LOCKDOWN_LIFT_KEY, &lockdown_lift_sysrq_op);
> +	}
> +	return 0;
> +}
> +
> +late_initcall(lockdown_lift_sysrq);
> +
> +#endif /* CONFIG_ALLOW_LOCKDOWN_LIFT_BY_KEY */

On non-x86 platforms (tested on powerpc) this fails to build with:

security/lock_down.c: In function ‘lockdown_lift_sysrq’:
security/lock_down.c:100:40: error: ‘LOCKDOWN_LIFT_KEY’ undeclared (first use in this function)
   lockdown_lift_sysrq_op.help_msg[5] = LOCKDOWN_LIFT_KEY;
                                        ^~~~~~~~~~~~~~~~~
security/lock_down.c:100:40: note: each undeclared identifier is reported only once for each function it appears in

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 02/27] Add a SysRq option to lift kernel lockdown

[David Howells]

Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> wrote:

> On non-x86 platforms (tested on powerpc) this fails to build with:
> 
> security/lock_down.c: In function ‘lockdown_lift_sysrq’:
> security/lock_down.c:100:40: error: ‘LOCKDOWN_LIFT_KEY’ undeclared (first use in this function)
>    lockdown_lift_sysrq_op.help_msg[5] = LOCKDOWN_LIFT_KEY;
>                                         ^~~~~~~~~~~~~~~~~
> security/lock_down.c:100:40: note: each undeclared identifier is reported only once for each function it appears in

I've added an arch dependency in the Kconfig file in my local branch.  I'll
try to get it pushed again.

David

[PATCH 03/27] Enforce module signatures if the kernel is locked down

[David Howells]

If the kernel is locked down, require that all modules have valid
signatures that we can verify.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 kernel/module.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..3d9a3270c179 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2781,7 +2781,8 @@ static int module_sig_check(struct load_info *info, int flags)
 	}
 
 	/* Not having a signature is only an error if we're strict. */
-	if (err == -ENOKEY && !sig_enforce)
+	if (err == -ENOKEY && !sig_enforce &&
+	    !kernel_is_locked_down("Loading of unsigned modules"))
 		err = 0;
 
 	return err;

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[joeyli]

Hi David,

Thanks for you send our this series.

On Thu, Oct 19, 2017 at 03:50:55PM +0100, David Howells wrote:
> If the kernel is locked down, require that all modules have valid
> signatures that we can verify.
> 
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I have reviewed and tested this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>

Thanks a lot!
Joey Lee

> ---
> 
>  kernel/module.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index de66ec825992..3d9a3270c179 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2781,7 +2781,8 @@ static int module_sig_check(struct load_info *info, int flags)
>  	}
>  
>  	/* Not having a signature is only an error if we're strict. */
> -	if (err == -ENOKEY && !sig_enforce)
> +	if (err == -ENOKEY && !sig_enforce &&
> +	    !kernel_is_locked_down("Loading of unsigned modules"))
>  		err = 0;
>  
>  	return err;
> 
> --
> 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

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[James Morris]

On Thu, 19 Oct 2017, David Howells wrote:

> If the kernel is locked down, require that all modules have valid
> signatures that we can verify.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<james.l.morris@oracle.com>

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[Mimi Zohar]

On Thu, 2017-10-19 at 15:50 +0100, David Howells wrote:
> If the kernel is locked down, require that all modules have valid
> signatures that we can verify.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  kernel/module.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index de66ec825992..3d9a3270c179 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2781,7 +2781,8 @@ static int module_sig_check(struct load_info *info, int flags)
>  	}
> 
>  	/* Not having a signature is only an error if we're strict. */
> -	if (err == -ENOKEY && !sig_enforce)
> +	if (err == -ENOKEY && !sig_enforce &&
> +	    !kernel_is_locked_down("Loading of unsigned modules"))
 
This kernel_is_locked_down() check is being called for both the
original and new module_load syscalls.  We need to be able
differentiate them.  This is fine for the original syscall, but for
the new syscall we would need an additional IMA check -
!is_ima_appraise_enabled().

Mimi
 
>  		err = 0;
> 
>  	return err;

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> This kernel_is_locked_down() check is being called for both the
> original and new module_load syscalls.  We need to be able
> differentiate them.  This is fine for the original syscall, but for
> the new syscall we would need an additional IMA check -
> !is_ima_appraise_enabled().

IMA can only be used with finit_module()?

David

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[Mimi Zohar]

[Corrected Matthew Garrett's email address.  Cc'ed Bruno Meneguele]

On Mon, 2017-10-30 at 17:00 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > This kernel_is_locked_down() check is being called for both the
> > original and new module_load syscalls.  We need to be able
> > differentiate them.  This is fine for the original syscall, but for
> > the new syscall we would need an additional IMA check -
> > !is_ima_appraise_enabled().
> 
> IMA can only be used with finit_module()?

Yes, without the file descriptor, IMA-appraisal can't access the
xattrs. 

You should really look at Bruno's patches, which are in my next
branch:

8168913c50d5 "ima: check signature enforcement against cmdline param instead of CONFIG"
404090509894 module: export module signature enforcement status

Can we get an Ack on the module one?

Mimi

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[David Howells]

Hi Mimi,

I've altered this patch to allow for IMA appraisal on finit_module().  See the
attached.

David
---
commit c0d5336356004e7543314e388755a00e725521da
Author: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Wed May 24 14:56:01 2017 +0100

    Enforce module signatures if the kernel is locked down
    
    If the kernel is locked down, require that all modules have valid
    signatures that we can verify or that IMA can validate the file.
    
    Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
    Reviewed-by: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

diff --git a/kernel/module.c b/kernel/module.c
index de66ec825992..0ce29c8aa75a 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -64,6 +64,7 @@
 #include <linux/bsearch.h>
 #include <linux/dynamic_debug.h>
 #include <linux/audit.h>
+#include <linux/ima.h>
 #include <uapi/linux/module.h>
 #include "module-internal.h"
 
@@ -2757,7 +2758,8 @@ static inline void kmemleak_load_module(const struct module *mod,
 #endif
 
 #ifdef CONFIG_MODULE_SIG
-static int module_sig_check(struct load_info *info, int flags)
+static int module_sig_check(struct load_info *info, int flags,
+			    bool can_do_ima_check)
 {
 	int err = -ENOKEY;
 	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
@@ -2781,13 +2783,16 @@ static int module_sig_check(struct load_info *info, int flags)
 	}
 
 	/* Not having a signature is only an error if we're strict. */
-	if (err == -ENOKEY && !sig_enforce)
+	if (err == -ENOKEY && !sig_enforce &&
+	    (!can_do_ima_check || !is_ima_appraise_enabled()) &&
+	    !kernel_is_locked_down("Loading of unsigned modules"))
 		err = 0;
 
 	return err;
 }
 #else /* !CONFIG_MODULE_SIG */
-static int module_sig_check(struct load_info *info, int flags)
+static int module_sig_check(struct load_info *info, int flags,
+			    bool can_do_ima_check)
 {
 	return 0;
 }
@@ -3630,13 +3635,13 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname,
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static int load_module(struct load_info *info, const char __user *uargs,
-		       int flags)
+		       int flags, bool can_do_ima_check)
 {
 	struct module *mod;
 	long err;
 	char *after_dashes;
 
-	err = module_sig_check(info, flags);
+	err = module_sig_check(info, flags, can_do_ima_check);
 	if (err)
 		goto free_copy;
 
@@ -3830,7 +3835,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 	if (err)
 		return err;
 
-	return load_module(&info, uargs, 0);
+	return load_module(&info, uargs, 0, false);
 }
 
 SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
@@ -3857,7 +3862,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
 	info.hdr = hdr;
 	info.len = size;
 
-	return load_module(&info, uargs, flags);
+	return load_module(&info, uargs, flags, true);
 }
 
 static inline int within(unsigned long addr, void *start, unsigned long size)

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[Mimi Zohar]

On Thu, 2017-11-02 at 17:22 +0000, David Howells wrote:

>  #ifdef CONFIG_MODULE_SIG
> -static int module_sig_check(struct load_info *info, int flags)
> +static int module_sig_check(struct load_info *info, int flags,
> +			    bool can_do_ima_check)
>  {
>  	int err = -ENOKEY;
>  	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> @@ -2781,13 +2783,16 @@ static int module_sig_check(struct load_info *info, int flags)
>  	}
>  
>  	/* Not having a signature is only an error if we're strict. */
> -	if (err == -ENOKEY && !sig_enforce)
> +	if (err == -ENOKEY && !sig_enforce &&
> +	    (!can_do_ima_check || !is_ima_appraise_enabled()) &&
> +	    !kernel_is_locked_down("Loading of unsigned modules"))

By this point, IMA-appraisal has already verified the kernel module
signature back in kernel_read_file_from_fd(), if it was required.
 Having a key with which to verify the appended signature or requiring
an appended signature, should not be required as well.

Mimi

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[David Howells]

Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:

> By this point, IMA-appraisal has already verified the kernel module
> signature back in kernel_read_file_from_fd(), if it was required.
>  Having a key with which to verify the appended signature or requiring
> an appended signature, should not be required as well.

I guess I don't need to put in any support for IMA here, then, and you've
taken care of it in your patchset such that it won't actually go into
module_sig_check() in that case (or will at least return immediately).

David

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[Mimi Zohar]

On Thu, 2017-11-02 at 21:30 +0000, David Howells wrote:
> Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> 
> > By this point, IMA-appraisal has already verified the kernel module
> > signature back in kernel_read_file_from_fd(), if it was required.
> >  Having a key with which to verify the appended signature or requiring
> > an appended signature, should not be required as well.
> 
> I guess I don't need to put in any support for IMA here, then, and you've
> taken care of it in your patchset such that it won't actually go into
> module_sig_check() in that case (or will at least return immediately).

Right, it would never get here if the IMA signature verification
fails.  If sig_enforce is not enabled, then it will also work.  So the
only case is if sig_enforced is enabled and there is no key.

eg.
         else if (can_do_ima_check && is_ima_appraise_enabled())
                err = 0;

Mimi 

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[David Howells]

Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:

> Right, it would never get here if the IMA signature verification
> fails.  If sig_enforce is not enabled, then it will also work.  So the
> only case is if sig_enforced is enabled and there is no key.
> 
> eg.
>          else if (can_do_ima_check && is_ima_appraise_enabled())
>                 err = 0;

I'm not sure where you want to put that, but I can't just do this:

	/* Not having a signature is only an error if we're strict. */
	if (err == -ENOKEY && !sig_enforce &&
	    (!can_do_ima_check || !is_ima_appraise_enabled()) &&
	    !kernel_is_locked_down("Loading of unsigned modules"))
		err = 0;
	else if (can_do_ima_check && is_ima_appraise_enabled())
		err = 0;

because that'll print out a message in lockdown mode saying that you're not
allowed to do that and then maybe do it anyway.

David

Re: [PATCH 03/27] Enforce module signatures if the kernel is locked down

[Mimi Zohar]

On Thu, 2017-11-02 at 22:01 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > Right, it would never get here if the IMA signature verification
> > fails.  If sig_enforce is not enabled, then it will also work.  So the
> > only case is if sig_enforced is enabled and there is no key.
> > 
> > eg.
> >          else if (can_do_ima_check && is_ima_appraise_enabled())
> >                 err = 0;
> 
> I'm not sure where you want to put that, but I can't just do this:
> 
> 	/* Not having a signature is only an error if we're strict. */
> 	if (err == -ENOKEY && !sig_enforce &&
> 	    (!can_do_ima_check || !is_ima_appraise_enabled()) &&

The above IMA checks aren't needed here.

> 	    !kernel_is_locked_down("Loading of unsigned modules"))
> 		err = 0;
> 	else if (can_do_ima_check && is_ima_appraise_enabled())
> 		err = 0;
> 
> because that'll print out a message in lockdown mode saying that you're not
> allowed to do that and then maybe do it anyway.

Then at least for now, document that even though kernel modules might
be signed and verified by IMA-appraisal, that in lockdown mode they
also require an appended signature.

Mimi

[PATCH 04/27] Restrict /dev/mem and /dev/kmem when the kernel is locked down

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

Allowing users to write to address space makes it possible for the kernel to
be subverted, avoiding module loading restrictions.  Prevent this when the
kernel has been locked down.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/char/mem.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 593a8818aca9..b7c36898b689 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -179,6 +179,9 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
 	if (p != *ppos)
 		return -EFBIG;
 
+	if (kernel_is_locked_down("/dev/mem"))
+		return -EPERM;
+
 	if (!valid_phys_addr_range(p, count))
 		return -EFAULT;
 
@@ -540,6 +543,9 @@ static ssize_t write_kmem(struct file *file, const char __user *buf,
 	char *kbuf; /* k-addr because vwrite() takes vmlist_lock rwlock */
 	int err = 0;
 
+	if (kernel_is_locked_down("/dev/kmem"))
+		return -EPERM;
+
 	if (p < (unsigned long) high_memory) {
 		unsigned long to_write = min_t(unsigned long, count,
 					       (unsigned long)high_memory - p);

Re: [PATCH 04/27] Restrict /dev/mem and /dev/kmem when the kernel is locked down

[joeyli]

Hi David,

Thanks for you send out this series.

On Thu, Oct 19, 2017 at 03:51:02PM +0100, David Howells wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> Allowing users to write to address space makes it possible for the kernel to
> be subverted, avoiding module loading restrictions.  Prevent this when the
> kernel has been locked down.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>

I have reviewed and tested this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks a lot!
Joey Lee

> ---
> 
>  drivers/char/mem.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 593a8818aca9..b7c36898b689 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -179,6 +179,9 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
>  	if (p != *ppos)
>  		return -EFBIG;
>  
> +	if (kernel_is_locked_down("/dev/mem"))
> +		return -EPERM;
> +
>  	if (!valid_phys_addr_range(p, count))
>  		return -EFAULT;
>  
> @@ -540,6 +543,9 @@ static ssize_t write_kmem(struct file *file, const char __user *buf,
>  	char *kbuf; /* k-addr because vwrite() takes vmlist_lock rwlock */
>  	int err = 0;
>  
> +	if (kernel_is_locked_down("/dev/kmem"))
> +		return -EPERM;
> +
>  	if (p < (unsigned long) high_memory) {
>  		unsigned long to_write = min_t(unsigned long, count,
>  					       (unsigned long)high_memory - p);
> 
> --
> 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

Re: [PATCH 04/27] Restrict /dev/mem and /dev/kmem when the kernel is locked down

[James Morris]

On Thu, 19 Oct 2017, David Howells wrote:

> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> Allowing users to write to address space makes it possible for the kernel to
> be subverted, avoiding module loading restrictions.  Prevent this when the
> kernel has been locked down.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<james.l.morris@oracle.com>

[PATCH 05/27] kexec: Disable at runtime if the kernel is locked down

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

kexec permits the loading and execution of arbitrary code in ring 0, which
is something that lock-down is meant to prevent. It makes sense to disable
kexec in this situation.

This does not affect kexec_file_load() which can check for a signature on the
image to be booted.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Dave Young <dyoung@redhat.com>
cc: kexec@lists.infradead.org
---

 kernel/kexec.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index e62ec4dc6620..7dadfed9b676 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -202,6 +202,13 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
 		return -EPERM;
 
 	/*
+	 * kexec can be used to circumvent module loading restrictions, so
+	 * prevent loading in that case
+	 */
+	if (kernel_is_locked_down("kexec of unsigned images"))
+		return -EPERM;
+
+	/*
 	 * Verify we have a legal set of flags
 	 * This leaves us room for future extensions.
 	 */

Re: [PATCH 05/27] kexec: Disable at runtime if the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:51:09PM +0100, David Howells wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> kexec permits the loading and execution of arbitrary code in ring 0, which
> is something that lock-down is meant to prevent. It makes sense to disable
> kexec in this situation.
> 
> This does not affect kexec_file_load() which can check for a signature on the
> image to be booted.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Acked-by: Dave Young <dyoung@redhat.com>

I have reviewed and tested this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks a lot!
Joey Lee

> cc: kexec@lists.infradead.org
> ---
> 
>  kernel/kexec.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index e62ec4dc6620..7dadfed9b676 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -202,6 +202,13 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
>  		return -EPERM;
>  
>  	/*
> +	 * kexec can be used to circumvent module loading restrictions, so
> +	 * prevent loading in that case
> +	 */
> +	if (kernel_is_locked_down("kexec of unsigned images"))
> +		return -EPERM;
> +
> +	/*
>  	 * Verify we have a legal set of flags
>  	 * This leaves us room for future extensions.
>  	 */
> 
> --
> 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

Re: [PATCH 05/27] kexec: Disable at runtime if the kernel is locked down

[James Morris]

On Thu, 19 Oct 2017, David Howells wrote:

> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> kexec permits the loading and execution of arbitrary code in ring 0, which
> is something that lock-down is meant to prevent. It makes sense to disable
> kexec in this situation.
> 
> This does not affect kexec_file_load() which can check for a signature on the
> image to be booted.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Acked-by: Dave Young <dyoung@redhat.com>
> cc: kexec@lists.infradead.org


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<james.l.morris@oracle.com>

[PATCH 06/27] Copy secure_boot flag in boot params across kexec reboot

[David Howells]

From: Dave Young <dyoung@redhat.com>

Kexec reboot in case secure boot being enabled does not keep the secure
boot mode in new kernel, so later one can load unsigned kernel via legacy
kexec_load.  In this state, the system is missing the protections provided
by secure boot.

Adding a patch to fix this by retain the secure_boot flag in original
kernel.

secure_boot flag in boot_params is set in EFI stub, but kexec bypasses the
stub.  Fixing this issue by copying secure_boot flag across kexec reboot.

Signed-off-by: Dave Young <dyoung@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: kexec@lists.infradead.org
---

 arch/x86/kernel/kexec-bzimage64.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index fb095ba0c02f..7d0fac5bcbbe 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -179,6 +179,7 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
 	if (efi_enabled(EFI_OLD_MEMMAP))
 		return 0;
 
+	params->secure_boot = boot_params.secure_boot;
 	ei->efi_loader_signature = current_ei->efi_loader_signature;
 	ei->efi_systab = current_ei->efi_systab;
 	ei->efi_systab_hi = current_ei->efi_systab_hi;

Re: [PATCH 06/27] Copy secure_boot flag in boot params across kexec reboot

[joeyli]

On Thu, Oct 19, 2017 at 03:51:20PM +0100, David Howells wrote:
> From: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Kexec reboot in case secure boot being enabled does not keep the secure
> boot mode in new kernel, so later one can load unsigned kernel via legacy
> kexec_load.  In this state, the system is missing the protections provided
> by secure boot.
> 
> Adding a patch to fix this by retain the secure_boot flag in original
> kernel.
> 
> secure_boot flag in boot_params is set in EFI stub, but kexec bypasses the
> stub.  Fixing this issue by copying secure_boot flag across kexec reboot.
> 
> Signed-off-by: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> cc: kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

I have reviewed and tested this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>

Thanks a lot!
Joey Lee

> ---
> 
>  arch/x86/kernel/kexec-bzimage64.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index fb095ba0c02f..7d0fac5bcbbe 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -179,6 +179,7 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  	if (efi_enabled(EFI_OLD_MEMMAP))
>  		return 0;
>  
> +	params->secure_boot = boot_params.secure_boot;
>  	ei->efi_loader_signature = current_ei->efi_loader_signature;
>  	ei->efi_systab = current_ei->efi_systab;
>  	ei->efi_systab_hi = current_ei->efi_systab_hi;
> 
> --
> 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 07/27] kexec_file: Disable at runtime if securelevel has been set

[David Howells]

From: Chun-Yi Lee <joeyli.kernel@gmail.com>

When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
through kexec_file systemcall if securelevel has been set.

This code was showed in Matthew's patch but not in git:
https://lkml.org/lkml/2015/3/13/778

Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Signed-off-by: Chun-Yi Lee <jlee@suse.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: kexec@lists.infradead.org
---

 kernel/kexec_file.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 9f48f4412297..ff6523f2dcc2 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -255,6 +255,13 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
 		return -EPERM;
 
+	/* Don't permit images to be loaded into trusted kernels if we're not
+	 * going to verify the signature on them
+	 */
+	if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
+	    kernel_is_locked_down("kexec of unsigned images"))
+		return -EPERM;
+
 	/* Make sure we have a legal set of flags */
 	if (flags != (flags & KEXEC_FILE_FLAGS))
 		return -EINVAL;

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[James Morris]

On Thu, 19 Oct 2017, David Howells wrote:

> From: Chun-Yi Lee <joeyli.kernel@gmail.com>
> 
> When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> through kexec_file systemcall if securelevel has been set.
> 
> This code was showed in Matthew's patch but not in git:
> https://lkml.org/lkml/2015/3/13/778
> 


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<james.l.morris@oracle.com>

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> From: Chun-Yi Lee <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> through kexec_file systemcall if securelevel has been set.

The patch title and description needs to be updated to refer to
lockdown, not securelevel.

As previously mentioned the last time these patches were posted, this
leaves out testing to see if the integrity subsystem is enabled.

Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
file signatures" was upstreamed.  An additional patch could force
these rules to be added to the custom policy, if lockdown is enabled.
 This and other patches in this series could then check to see if
is_ima_appraise_enabled() is true.

Mimi


> This code was showed in Matthew's patch but not in git:
> https://lkml.org/lkml/2015/3/13/778
> 
> Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
> Signed-off-by: Chun-Yi Lee <jlee-IBi9RG/b67k@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> cc: kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> ---
> 
>  kernel/kexec_file.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 9f48f4412297..ff6523f2dcc2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -255,6 +255,13 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
>  		return -EPERM;
> 
> +	/* Don't permit images to be loaded into trusted kernels if we're not
> +	 * going to verify the signature on them
> +	 */
> +	if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
> +	    kernel_is_locked_down("kexec of unsigned images"))
> +		return -EPERM;
> +
>  	/* Make sure we have a legal set of flags */
>  	if (flags != (flags & KEXEC_FILE_FLAGS))
>  		return -EINVAL;
> 
> --
> 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 07/27] kexec_file: Disable at runtime if securelevel has been set

[joeyli]

Hi Mimi,

Thank you for reviewing.

On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote:
> On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> > From: Chun-Yi Lee <joeyli.kernel@gmail.com>
> > 
> > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> > through kexec_file systemcall if securelevel has been set.
> 
> The patch title and description needs to be updated to refer to
> lockdown, not securelevel.
> 
> As previously mentioned the last time these patches were posted, this
> leaves out testing to see if the integrity subsystem is enabled.
> 
> Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
> file signatures" was upstreamed.  An additional patch could force
> these rules to be added to the custom policy, if lockdown is enabled.
>  This and other patches in this series could then check to see if
> is_ima_appraise_enabled() is true.
> 
> Mimi
>

I have updated the patch title and description, and I also added
is_ima_appraise_enabled() as the following. Is it good to you?

On the other hand, I am not good on IMA. I have traced the code path
in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE
doesn't show in selinux_kernel_read_file(). Where is the exact code
in IMA for checking the signature when loading crash kernel file? 

Thanks a lot!
Joey Lee
---

>From 274a2125132ba5aff49e4ccd167f52982732361f Mon Sep 17 00:00:00 2001
From: "Lee, Chun-Yi" <jlee@suse.com>
Date: Thu, 26 Oct 2017 15:24:50 +0800
Subject: [PATCH] kexec_file: The integrity must be checked when the kernel is
 locked down

When KEXEC_VERIFY_SIG and IMA appraise are not enabled, kernel should
not allow that the image to be loaded by kexec_file systemcall when the
kernel is locked down.

The original code was showed in Matthew's patch but not in the later
patch set:
    https://lkml.org/lkml/2015/3/13/778

Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 kernel/kexec_file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 9f48f44..b6dc218 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -255,6 +255,14 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
 		return -EPERM;
 
+	/* Don't permit images to be loaded into trusted kernels if we're not
+	 * going to check the integrity on them
+	 */
+	if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
+	    !is_ima_appraise_enabled() &&
+	    kernel_is_locked_down("kexec of unsigned images"))
+		return -EPERM;
+
 	/* Make sure we have a legal set of flags */
 	if (flags != (flags & KEXEC_FILE_FLAGS))
 		return -EINVAL;
-- 
2.6.2

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote:
> Hi Mimi,
> 
> Thank you for reviewing.
> 
> On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote:
> > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> > > From: Chun-Yi Lee <joeyli.kernel@gmail.com>
> > > 
> > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> > > through kexec_file systemcall if securelevel has been set.
> > 
> > The patch title and description needs to be updated to refer to
> > lockdown, not securelevel.
> > 
> > As previously mentioned the last time these patches were posted, this
> > leaves out testing to see if the integrity subsystem is enabled.
> > 
> > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
> > file signatures" was upstreamed.  An additional patch could force
> > these rules to be added to the custom policy, if lockdown is enabled.
> >  This and other patches in this series could then check to see if
> > is_ima_appraise_enabled() is true.
> > 
> > Mimi
> >
> 
> I have updated the patch title and description, and I also added
> is_ima_appraise_enabled() as the following. Is it good to you?

Yes, that works.  Thanks!  Remember is_ima_appraise_enabled() is
dependent on the "ima: require secure_boot rules in lockdown mode"
patch - http://kernsec.org/pipermail/linux-security-module-archive/201
7-October/003910.html.

The IMA "secure_boot" policy can be specified on the boot command line
as ima_policy="secure_boot".  It requires kernel modules, firmware,
kexec kernel image and the IMA custom policy to be signed.  In
lockdown mode, these rules are enabled by default and added to the
custom policy.

> On the other hand, I am not good on IMA. I have traced the code path
> in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE
> doesn't show in selinux_kernel_read_file(). Where is the exact code
> in IMA for checking the signature when loading crash kernel file?

kernel_read_file_from_fd() calls the security_kernel_read_file() and
security_kernel_post_read_file() hooks, which call ima_read_file() and
ima_post_read_file() respectively.

Mimi

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Thu, 2017-10-26 at 10:17 -0400, Mimi Zohar wrote:
> On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote:
> > Hi Mimi,
> > 
> > Thank you for reviewing.
> > 
> > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote:
> > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> > > > From: Chun-Yi Lee <joeyli.kernel@gmail.com>
> > > > 
> > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> > > > through kexec_file systemcall if securelevel has been set.
> > > 
> > > The patch title and description needs to be updated to refer to
> > > lockdown, not securelevel.
> > > 
> > > As previously mentioned the last time these patches were posted, this
> > > leaves out testing to see if the integrity subsystem is enabled.
> > > 
> > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
> > > file signatures" was upstreamed.  An additional patch could force
> > > these rules to be added to the custom policy, if lockdown is enabled.
> > >  This and other patches in this series could then check to see if
> > > is_ima_appraise_enabled() is true.
> > > 
> > > Mimi
> > >
> > 
> > I have updated the patch title and description, and I also added
> > is_ima_appraise_enabled() as the following. Is it good to you?
> 
> Yes, that works.  Thanks!  Remember is_ima_appraise_enabled() is
> dependent on the "ima: require secure_boot rules in lockdown mode"
> patch - http://kernsec.org/pipermail/linux-security-module-archive/201
> 7-October/003910.html.
> 
> The IMA "secure_boot" policy can be specified on the boot command line
> as ima_policy="secure_boot".  It requires kernel modules, firmware,
> kexec kernel image and the IMA custom policy to be signed.  In
> lockdown mode, these rules are enabled by default and added to the
> custom policy.
> 
> > On the other hand, I am not good on IMA. I have traced the code path
> > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE
> > doesn't show in selinux_kernel_read_file(). Where is the exact code
> > in IMA for checking the signature when loading crash kernel file?
> 
> kernel_read_file_from_fd() calls the security_kernel_read_file() and
> security_kernel_post_read_file() hooks, which call ima_read_file() and
> ima_post_read_file() respectively.

Hm, with "lockdown" enabled on the boot command line, I'm now able to
do the kexec load, but not the unload.  :/   After the kexec load with
the "--reuse-cmdline" option, the system reboots, but isn't in
"lockdown" mode.

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Thu, 2017-10-26 at 10:17 -0400, Mimi Zohar wrote:
> On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote:
> > Hi Mimi,
> > 
> > Thank you for reviewing.
> > 
> > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote:
> > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> > > > From: Chun-Yi Lee <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > 
> > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> > > > through kexec_file systemcall if securelevel has been set.
> > > 
> > > The patch title and description needs to be updated to refer to
> > > lockdown, not securelevel.
> > > 
> > > As previously mentioned the last time these patches were posted, this
> > > leaves out testing to see if the integrity subsystem is enabled.
> > > 
> > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
> > > file signatures" was upstreamed.  An additional patch could force
> > > these rules to be added to the custom policy, if lockdown is enabled.
> > >  This and other patches in this series could then check to see if
> > > is_ima_appraise_enabled() is true.
> > > 
> > > Mimi
> > >
> > 
> > I have updated the patch title and description, and I also added
> > is_ima_appraise_enabled() as the following. Is it good to you?
> 
> Yes, that works.  Thanks!  Remember is_ima_appraise_enabled() is
> dependent on the "ima: require secure_boot rules in lockdown mode"
> patch - http://kernsec.org/pipermail/linux-security-module-archive/201
> 7-October/003910.html.
> 
> The IMA "secure_boot" policy can be specified on the boot command line
> as ima_policy="secure_boot".  It requires kernel modules, firmware,
> kexec kernel image and the IMA custom policy to be signed.  In
> lockdown mode, these rules are enabled by default and added to the
> custom policy.
> 
> > On the other hand, I am not good on IMA. I have traced the code path
> > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE
> > doesn't show in selinux_kernel_read_file(). Where is the exact code
> > in IMA for checking the signature when loading crash kernel file?
> 
> kernel_read_file_from_fd() calls the security_kernel_read_file() and
> security_kernel_post_read_file() hooks, which call ima_read_file() and
> ima_post_read_file() respectively.

Hm, with "lockdown" enabled on the boot command line, I'm now able to
do the kexec load, but not the unload.  :/   After the kexec load with
the "--reuse-cmdline" option, the system reboots, but isn't in
"lockdown" mode.

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[joeyli]

On Fri, Oct 27, 2017 at 03:32:26PM -0400, Mimi Zohar wrote:
> On Thu, 2017-10-26 at 10:17 -0400, Mimi Zohar wrote:
> > On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote:
> > > Hi Mimi,
> > > 
> > > Thank you for reviewing.
> > > 
> > > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote:
> > > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> > > > > From: Chun-Yi Lee <joeyli.kernel@gmail.com>
> > > > > 
> > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> > > > > through kexec_file systemcall if securelevel has been set.
> > > > 
> > > > The patch title and description needs to be updated to refer to
> > > > lockdown, not securelevel.
> > > > 
> > > > As previously mentioned the last time these patches were posted, this
> > > > leaves out testing to see if the integrity subsystem is enabled.
> > > > 
> > > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
> > > > file signatures" was upstreamed.  An additional patch could force
> > > > these rules to be added to the custom policy, if lockdown is enabled.
> > > >  This and other patches in this series could then check to see if
> > > > is_ima_appraise_enabled() is true.
> > > > 
> > > > Mimi
> > > >
> > > 
> > > I have updated the patch title and description, and I also added
> > > is_ima_appraise_enabled() as the following. Is it good to you?
> > 
> > Yes, that works.  Thanks!  Remember is_ima_appraise_enabled() is
> > dependent on the "ima: require secure_boot rules in lockdown mode"
> > patch - http://kernsec.org/pipermail/linux-security-module-archive/201
> > 7-October/003910.html.
> > 
> > The IMA "secure_boot" policy can be specified on the boot command line
> > as ima_policy="secure_boot".  It requires kernel modules, firmware,
> > kexec kernel image and the IMA custom policy to be signed.  In
> > lockdown mode, these rules are enabled by default and added to the
> > custom policy.
> > 
> > > On the other hand, I am not good on IMA. I have traced the code path
> > > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE
> > > doesn't show in selinux_kernel_read_file(). Where is the exact code
> > > in IMA for checking the signature when loading crash kernel file?
> > 
> > kernel_read_file_from_fd() calls the security_kernel_read_file() and
> > security_kernel_post_read_file() hooks, which call ima_read_file() and
> > ima_post_read_file() respectively.
> 
> Hm, with "lockdown" enabled on the boot command line, I'm now able to
> do the kexec load, but not the unload.  :/   After the kexec load with

I have tried on Qemu with OVMF, I can load and unload second kernel by
kexec tool (on openSUSE is in kexec-tools RPM):  

# kexec -u -s

I add -s for using kexec-load-file, and I signed kernel by pesign.

> the "--reuse-cmdline" option, the system reboots, but isn't in
> "lockdown" mode.
>

Either enabling secure boot in EFI firmware or using _lockdown_ kernel
parameter, the second kernel can be locked down on my OVMF VM.

I used following commands:

# kexec -s -l /boot/vmlinuz-4.14.0-rc2-default+ --append="$(cat /proc/cmdline)" --initrd=/boot/initrd-4.14.0-rc2-default+
# umount -a; mount -o remount,ro /
# kexec -e

The kernel source is from David's linux-fs git with lockdown-20171026 tag.
The kernel is also signed by pesign.

Regards
Joey Lee 

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Sat, 2017-10-28 at 16:34 +0800, joeyli wrote:
> On Fri, Oct 27, 2017 at 03:32:26PM -0400, Mimi Zohar wrote:
> > On Thu, 2017-10-26 at 10:17 -0400, Mimi Zohar wrote:
> > > On Thu, 2017-10-26 at 15:42 +0800, joeyli wrote:
> > > > Hi Mimi,
> > > > 
> > > > Thank you for reviewing.
> > > > 
> > > > On Mon, Oct 23, 2017 at 11:54:43AM -0400, Mimi Zohar wrote:
> > > > > On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> > > > > > From: Chun-Yi Lee <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > > 
> > > > > > When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> > > > > > through kexec_file systemcall if securelevel has been set.
> > > > > 
> > > > > The patch title and description needs to be updated to refer to
> > > > > lockdown, not securelevel.
> > > > > 
> > > > > As previously mentioned the last time these patches were posted, this
> > > > > leaves out testing to see if the integrity subsystem is enabled.
> > > > > 
> > > > > Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
> > > > > file signatures" was upstreamed.  An additional patch could force
> > > > > these rules to be added to the custom policy, if lockdown is enabled.
> > > > >  This and other patches in this series could then check to see if
> > > > > is_ima_appraise_enabled() is true.
> > > > > 
> > > > > Mimi
> > > > >
> > > > 
> > > > I have updated the patch title and description, and I also added
> > > > is_ima_appraise_enabled() as the following. Is it good to you?
> > > 
> > > Yes, that works.  Thanks!  Remember is_ima_appraise_enabled() is
> > > dependent on the "ima: require secure_boot rules in lockdown mode"
> > > patch - http://kernsec.org/pipermail/linux-security-module-archive/201
> > > 7-October/003910.html.
> > > 
> > > The IMA "secure_boot" policy can be specified on the boot command line
> > > as ima_policy="secure_boot".  It requires kernel modules, firmware,
> > > kexec kernel image and the IMA custom policy to be signed.  In
> > > lockdown mode, these rules are enabled by default and added to the
> > > custom policy.
> > > 
> > > > On the other hand, I am not good on IMA. I have traced the code path
> > > > in kimage_file_prepare_segments(). Looks that the READING_KEXEC_IMAGE
> > > > doesn't show in selinux_kernel_read_file(). Where is the exact code
> > > > in IMA for checking the signature when loading crash kernel file?
> > > 
> > > kernel_read_file_from_fd() calls the security_kernel_read_file() and
> > > security_kernel_post_read_file() hooks, which call ima_read_file() and
> > > ima_post_read_file() respectively.
> > 
> > Hm, with "lockdown" enabled on the boot command line, I'm now able to
> > do the kexec load, but not the unload.  :/   After the kexec load with
> 
> I have tried on Qemu with OVMF, I can load and unload second kernel by
> kexec tool (on openSUSE is in kexec-tools RPM):  
> 
> # kexec -u -s

Thanks, I left off the "-s" option, causing it to fail.  This is the
correct behavior.  So both with/without the "-s" option are working
properly.

> I add -s for using kexec-load-file, and I signed kernel by pesign.
> 
> > the "--reuse-cmdline" option, the system reboots, but isn't in
> > "lockdown" mode.
> >
> 
> Either enabling secure boot in EFI firmware or using _lockdown_ kernel
> parameter, the second kernel can be locked down on my OVMF VM.
> 
> I used following commands:
> 
> # kexec -s -l /boot/vmlinuz-4.14.0-rc2-default+ --append="$(cat /proc/cmdline)" --initrd=/boot/initrd-4.14.0-rc2-default+
> # umount -a; mount -o remount,ro /I'
> # kexec -e
> 
> The kernel source is from David's linux-fs git with lockdown-20171026 tag.
> The kernel is also signed by pesign.

Yes, based on the patches in David's tree, "lockdown" is being carried
to the target OS properly.

Mimi

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> Yes, that works.  Thanks!  Remember is_ima_appraise_enabled() is
> dependent on the "ima: require secure_boot rules in lockdown mode"
> patch - http://kernsec.org/pipermail/linux-security-module-archive/201
> 7-October/003910.html.

What happens if the file in question is being accessed from a filesystem that
doesn't have xattrs and doesn't provide support for appraisal?  Is it rejected
outright or just permitted?

David

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Mon, 2017-10-30 at 09:00 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > Yes, that works.  Thanks!  Remember is_ima_appraise_enabled() is
> > dependent on the "ima: require secure_boot rules in lockdown mode"
> > patch - http://kernsec.org/pipermail/linux-security-module-archive/201
> > 7-October/003910.html.
> 
> What happens if the file in question is being accessed from a filesystem that
> doesn't have xattrs and doesn't provide support for appraisal?  Is it rejected
> outright or just permitted?

IMA-appraisal returns -EACCES for any error, including lack of xattr
support.

Thiago Bauermann posted the "Appended signatures support for IMA
appraisal" patch set.  This patch set allows the current kernel module
appended signature format to be used for verifying the kernel image.
 Once that patch set is upstreamed, we'll be able to update the IMA
"secure_boot" policy to permit appended signatures.

Mimi

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[David Howells]

joeyli <jlee@suse.com> wrote:

> +	if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
> +	    !is_ima_appraise_enabled() &&
> +	    kernel_is_locked_down("kexec of unsigned images"))

This doesn't seem right.  It seems that you can then kexec unsigned images
into a locked-down kernel if IMA appraise is enabled.

I think the commit message needs expansion as to why it's okay.  Can you also
do it as an additional patch rather than altering the original IMA-less patch
7?

David

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

[Cc'ing Matthew Garrett]

On Thu, 2017-10-26 at 16:02 +0100, David Howells wrote:
> joeyli <jlee-IBi9RG/b67k@public.gmane.org> wrote:
> 
> > +	if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
> > +	    !is_ima_appraise_enabled() &&
> > +	    kernel_is_locked_down("kexec of unsigned images"))
> 
> This doesn't seem right.  It seems that you can then kexec unsigned images
> into a locked-down kernel if IMA appraise is enabled.

Huh?!  With the "secure_boot" policy enabled on the boot command line,
IMA-appraisal would verify the kexec kernel image, firmware, kernel
modules, and custom IMA policy signatures.  With the "ima: require
secure_boot rules in lockdown mode" patch, the "lockdown" mode would
enable IMA-appraisal's secure_boot policy, without requiring the boot
command line option.  It would also add the secure_boot rules to the
custom policy, so that if the builtin policy is replaced with a custom
policy, the "secure_boot" policy would still be enforced.

Other patches in this patch series need to be updated as well to check
if IMA-appraisal is enabled.

Mimi

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> Huh?!  With the "secure_boot" policy enabled on the boot command line,
> IMA-appraisal would verify the kexec kernel image, firmware, kernel
> modules, and custom IMA policy signatures.

What happens if the "secure_boot" policy isn't enabled on the boot command
line?  Can you sum up both cases in a paragraph I can add to the patch
description?

> Other patches in this patch series need to be updated as well to check
> if IMA-appraisal is enabled.

Which exactly?  I've added your "!is_ima_appraise_enabled() &&" line to
kexec_file() and module_sig_check().  Anything else?

David

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Mon, 2017-10-30 at 15:49 +0000, David Howells wrote:
> Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> 
> > Huh?!  With the "secure_boot" policy enabled on the boot command line,
> > IMA-appraisal would verify the kexec kernel image, firmware, kernel
> > modules, and custom IMA policy signatures.
> 
> What happens if the "secure_boot" policy isn't enabled on the boot command
> line?  Can you sum up both cases in a paragraph I can add to the patch
> description?

The other patch automatically enables "secure_boot" for lockdown mode.
So there is no need to specify "secure_boot" on the boot command line.
 Reordering the patches so that the other patch comes before any call
to is_ima_appraise_enabled() will simplify this patch description.

> > Other patches in this patch series need to be updated as well to check
> > if IMA-appraisal is enabled.
> 
> Which exactly?  I've added your "!is_ima_appraise_enabled() &&" line to
> kexec_file() and module_sig_check().  Anything else?

load_module(), which calls module_sig_check(), is called by both the
old and new kernel module syscalls.  IMA is only on the new syscall.
 Did you differentiate between the kernel module syscalls?

There doesn't seem to be any other patches affected.  That said, the
IMA "secure_boot" policy is more stringent than what you have without
it.  For example, with the "secure_boot" policy enabled, firwmware
needs to be signed as well.  At some point, we'll want to also require
the initramfs be signed as well.

Both methods work independently of each other, but there needs to be
better coordination for when both methods are enabled at the same time
(eg. are both signatures required?).

For testing purposes, you can use the same certs/signing_key to sign
the kexec image, kernel modules and firmware, by loading the
signing_key on the .ima keyring.  Using evmctl, sign the files
(eg. evmctl ima_sign -a sha256 -k certs/signing_key.pem  --imasig
/boot/<vmlinuz>).

Mimi

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> At some point, we'll want to also require the initramfs be signed as well.

That could be tricky.  In Fedora, at least, that's assembled on the fly to
include just the drivers you need to be able to mount your root fs and find
the rest of your modules.  (Unless you mean just for the installer)

David

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[David Howells]

Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:

> The patch title and description needs to be updated to refer to
> lockdown, not securelevel.

Fixed, thanks.

> An additional patch could force these rules to be added to the custom
> policy, if lockdown is enabled.

I'll have a look at your patch, though at this point I'm leaning towards
passing the current series to James for security/next and then passing your
patch along afterwards, if that's okay with you.  It should still get in the
next merge window if that's the case.

David

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[David Howells]

Hi Mimi,

I've adjusted the patch as below.

David
---
commit bf33218ad2bf04f1b92f5c32499ab906f107864c
Author: Chun-Yi Lee <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date:   Wed May 24 14:56:03 2017 +0100

    kexec_file: Restrict at runtime if the kernel is locked down
    
    When KEXEC_VERIFY_SIG is not enabled, kernel should not load images through
    kexec_file systemcall if the kernel is locked down unless IMA can be used
    to validate the image.
    
    This code was showed in Matthew's patch but not in git:
    https://lkml.org/lkml/2015/3/13/778
    
    Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
    Signed-off-by: Chun-Yi Lee <jlee-IBi9RG/b67k@public.gmane.org>
    Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    Reviewed-by: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
    cc: kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 9f48f4412297..3ba28fc3fab0 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -255,6 +255,14 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
 		return -EPERM;
 
+	/* Don't permit images to be loaded into trusted kernels if we're not
+	 * going to verify the signature on them
+	 */
+	if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
+	    !is_ima_appraise_enabled() &&
+	    kernel_is_locked_down("kexec of unsigned images"))
+		return -EPERM;
+
 	/* Make sure we have a legal set of flags */
 	if (flags != (flags & KEXEC_FILE_FLAGS))
 		return -EINVAL;

[PATCH 08/27] hibernate: Disable when the kernel is locked down

[David Howells]

From: Josh Boyer <jwboyer@fedoraproject.org>

There is currently no way to verify the resume image when returning
from hibernate.  This might compromise the signed modules trust model,
so until we can work with signed hibernate images we disable it when the
kernel is locked down.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-pm@vger.kernel.org
---

 kernel/power/hibernate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index a5c36e9c56a6..f2eafefeec50 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -70,7 +70,7 @@ static const struct platform_hibernation_ops *hibernation_ops;
 
 bool hibernation_available(void)
 {
-	return (nohibernate == 0);
+	return nohibernate == 0 && !kernel_is_locked_down("Hibernation");
 }
 
 /**

Re: [PATCH 08/27] hibernate: Disable when the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:51:34PM +0100, David Howells wrote:
> From: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
> 
> There is currently no way to verify the resume image when returning
> from hibernate.  This might compromise the signed modules trust model,
> so until we can work with signed hibernate images we disable it when the
> kernel is locked down.
> 
> 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-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

I have reviewed and tested this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>

Thanks a lot!
Joey Lee

> ---
> 
>  kernel/power/hibernate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index a5c36e9c56a6..f2eafefeec50 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -70,7 +70,7 @@ static const struct platform_hibernation_ops *hibernation_ops;
>  
>  bool hibernation_available(void)
>  {
> -	return (nohibernate == 0);
> +	return nohibernate == 0 && !kernel_is_locked_down("Hibernation");
>  }
>  
>  /**
> 
> --
> 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 09/27] uswsusp: Disable when the kernel is locked down

[David Howells]

From: Matthew Garrett <mjg59@srcf.ucam.org>

uswsusp allows a user process to dump and then restore kernel state, which
makes it possible to modify the running kernel.  Disable this if the kernel
is locked down.

Signed-off-by: Matthew Garrett <mjg59@srcf.ucam.org>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-pm@vger.kernel.org
---

 kernel/power/user.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index 22df9f7ff672..678ade9decfe 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -52,6 +52,9 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	if (!hibernation_available())
 		return -EPERM;
 
+	if (kernel_is_locked_down("/dev/snapshot"))
+		return -EPERM;
+
 	lock_system_sleep();
 
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {

Re: [PATCH 09/27] uswsusp: Disable when the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:51:42PM +0100, David Howells wrote:
> From: Matthew Garrett <mjg59@srcf.ucam.org>
> 
> uswsusp allows a user process to dump and then restore kernel state, which
> makes it possible to modify the running kernel.  Disable this if the kernel
> is locked down.
> 
> Signed-off-by: Matthew Garrett <mjg59@srcf.ucam.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linux-pm@vger.kernel.org

I have reviewed and tested this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks a lot!
Joey Lee

> ---
> 
>  kernel/power/user.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index 22df9f7ff672..678ade9decfe 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -52,6 +52,9 @@ static int snapshot_open(struct inode *inode, struct file *filp)
>  	if (!hibernation_available())
>  		return -EPERM;
>  
> +	if (kernel_is_locked_down("/dev/snapshot"))
> +		return -EPERM;
> +
>  	lock_system_sleep();
>  
>  	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
> 
> --
> 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

Re: [PATCH 09/27] uswsusp: Disable when the kernel is locked down

[James Morris]

On Thu, 19 Oct 2017, David Howells wrote:

> From: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
> 
> uswsusp allows a user process to dump and then restore kernel state, which
> makes it possible to modify the running kernel.  Disable this if the kernel
> is locked down.
> 
> Signed-off-by: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>


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

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

[PATCH 10/27] PCI: Lock down BAR access when the kernel is locked down

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

Any hardware that can potentially generate DMA has to be locked down in
order to avoid it being possible for an attacker to modify kernel code,
allowing them to circumvent disabled module loading or module signing.
Default to paranoid - in future we can potentially relax this for
sufficiently IOMMU-isolated devices.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
cc: linux-pci@vger.kernel.org
---

 drivers/pci/pci-sysfs.c |    9 +++++++++
 drivers/pci/proc.c      |    9 ++++++++-
 drivers/pci/syscall.c   |    3 ++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 1eecfa301f7f..e1a3b0e765c2 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -881,6 +881,9 @@ static ssize_t pci_write_config(struct file *filp, struct kobject *kobj,
 	loff_t init_off = off;
 	u8 *data = (u8 *) buf;
 
+	if (kernel_is_locked_down("Direct PCI access"))
+		return -EPERM;
+
 	if (off > dev->cfg_size)
 		return 0;
 	if (off + count > dev->cfg_size) {
@@ -1175,6 +1178,9 @@ static int pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
 	enum pci_mmap_state mmap_type;
 	struct resource *res = &pdev->resource[bar];
 
+	if (kernel_is_locked_down("Direct PCI access"))
+		return -EPERM;
+
 	if (res->flags & IORESOURCE_MEM && iomem_is_exclusive(res->start))
 		return -EINVAL;
 
@@ -1255,6 +1261,9 @@ static ssize_t pci_write_resource_io(struct file *filp, struct kobject *kobj,
 				     struct bin_attribute *attr, char *buf,
 				     loff_t off, size_t count)
 {
+	if (kernel_is_locked_down("Direct PCI access"))
+		return -EPERM;
+
 	return pci_resource_io(filp, kobj, attr, buf, off, count, true);
 }
 
diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index 098360d7ff81..a6c53d855daa 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -116,6 +116,9 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
 	int size = dev->cfg_size;
 	int cnt;
 
+	if (kernel_is_locked_down("Direct PCI access"))
+		return -EPERM;
+
 	if (pos >= size)
 		return 0;
 	if (nbytes >= size)
@@ -195,6 +198,9 @@ static long proc_bus_pci_ioctl(struct file *file, unsigned int cmd,
 #endif /* HAVE_PCI_MMAP */
 	int ret = 0;
 
+	if (kernel_is_locked_down("Direct PCI access"))
+		return -EPERM;
+
 	switch (cmd) {
 	case PCIIOC_CONTROLLER:
 		ret = pci_domain_nr(dev->bus);
@@ -236,7 +242,8 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
 	struct pci_filp_private *fpriv = file->private_data;
 	int i, ret, write_combine = 0, res_bit = IORESOURCE_MEM;
 
-	if (!capable(CAP_SYS_RAWIO))
+	if (!capable(CAP_SYS_RAWIO) ||
+	    kernel_is_locked_down("Direct PCI access"))
 		return -EPERM;
 
 	if (fpriv->mmap_state == pci_mmap_io) {
diff --git a/drivers/pci/syscall.c b/drivers/pci/syscall.c
index 9bf993e1f71e..afa01cc3ceec 100644
--- a/drivers/pci/syscall.c
+++ b/drivers/pci/syscall.c
@@ -92,7 +92,8 @@ SYSCALL_DEFINE5(pciconfig_write, unsigned long, bus, unsigned long, dfn,
 	u32 dword;
 	int err = 0;
 
-	if (!capable(CAP_SYS_ADMIN))
+	if (!capable(CAP_SYS_ADMIN) ||
+	    kernel_is_locked_down("Direct PCI access"))
 		return -EPERM;
 
 	dev = pci_get_bus_and_slot(bus, dfn);

Re: [PATCH 10/27] PCI: Lock down BAR access when the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:51:49PM +0100, David Howells wrote:
> From: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> 
> Any hardware that can potentially generate DMA has to be locked down in
> order to avoid it being possible for an attacker to modify kernel code,
> allowing them to circumvent disabled module loading or module signing.
> Default to paranoid - in future we can potentially relax this for
> sufficiently IOMMU-isolated devices.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>

Thanks a lot!
Joey Lee

> cc: linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
> 
>  drivers/pci/pci-sysfs.c |    9 +++++++++
>  drivers/pci/proc.c      |    9 ++++++++-
>  drivers/pci/syscall.c   |    3 ++-
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 1eecfa301f7f..e1a3b0e765c2 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -881,6 +881,9 @@ static ssize_t pci_write_config(struct file *filp, struct kobject *kobj,
>  	loff_t init_off = off;
>  	u8 *data = (u8 *) buf;
>  
> +	if (kernel_is_locked_down("Direct PCI access"))
> +		return -EPERM;
> +
>  	if (off > dev->cfg_size)
>  		return 0;
>  	if (off + count > dev->cfg_size) {
> @@ -1175,6 +1178,9 @@ static int pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
>  	enum pci_mmap_state mmap_type;
>  	struct resource *res = &pdev->resource[bar];
>  
> +	if (kernel_is_locked_down("Direct PCI access"))
> +		return -EPERM;
> +
>  	if (res->flags & IORESOURCE_MEM && iomem_is_exclusive(res->start))
>  		return -EINVAL;
>  
> @@ -1255,6 +1261,9 @@ static ssize_t pci_write_resource_io(struct file *filp, struct kobject *kobj,
>  				     struct bin_attribute *attr, char *buf,
>  				     loff_t off, size_t count)
>  {
> +	if (kernel_is_locked_down("Direct PCI access"))
> +		return -EPERM;
> +
>  	return pci_resource_io(filp, kobj, attr, buf, off, count, true);
>  }
>  
> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index 098360d7ff81..a6c53d855daa 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -116,6 +116,9 @@ static ssize_t proc_bus_pci_write(struct file *file, const char __user *buf,
>  	int size = dev->cfg_size;
>  	int cnt;
>  
> +	if (kernel_is_locked_down("Direct PCI access"))
> +		return -EPERM;
> +
>  	if (pos >= size)
>  		return 0;
>  	if (nbytes >= size)
> @@ -195,6 +198,9 @@ static long proc_bus_pci_ioctl(struct file *file, unsigned int cmd,
>  #endif /* HAVE_PCI_MMAP */
>  	int ret = 0;
>  
> +	if (kernel_is_locked_down("Direct PCI access"))
> +		return -EPERM;
> +
>  	switch (cmd) {
>  	case PCIIOC_CONTROLLER:
>  		ret = pci_domain_nr(dev->bus);
> @@ -236,7 +242,8 @@ static int proc_bus_pci_mmap(struct file *file, struct vm_area_struct *vma)
>  	struct pci_filp_private *fpriv = file->private_data;
>  	int i, ret, write_combine = 0, res_bit = IORESOURCE_MEM;
>  
> -	if (!capable(CAP_SYS_RAWIO))
> +	if (!capable(CAP_SYS_RAWIO) ||
> +	    kernel_is_locked_down("Direct PCI access"))
>  		return -EPERM;
>  
>  	if (fpriv->mmap_state == pci_mmap_io) {
> diff --git a/drivers/pci/syscall.c b/drivers/pci/syscall.c
> index 9bf993e1f71e..afa01cc3ceec 100644
> --- a/drivers/pci/syscall.c
> +++ b/drivers/pci/syscall.c
> @@ -92,7 +92,8 @@ SYSCALL_DEFINE5(pciconfig_write, unsigned long, bus, unsigned long, dfn,
>  	u32 dword;
>  	int err = 0;
>  
> -	if (!capable(CAP_SYS_ADMIN))
> +	if (!capable(CAP_SYS_ADMIN) ||
> +	    kernel_is_locked_down("Direct PCI access"))
>  		return -EPERM;
>  
>  	dev = pci_get_bus_and_slot(bus, dfn);
> 
> --
> 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 11/27] x86: Lock down IO port access when the kernel is locked down

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

IO port access would permit users to gain access to PCI configuration
registers, which in turn (on a lot of hardware) give access to MMIO
register space. This would potentially permit root to trigger arbitrary
DMA, so lock it down by default.

This also implicitly locks down the KDADDIO, KDDELIO, KDENABIO and
KDDISABIO console ioctls.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
cc: x86@kernel.org
---

 arch/x86/kernel/ioport.c |    6 ++++--
 drivers/char/mem.c       |    2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 9c3cf0944bce..2c0f058651c5 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -30,7 +30,8 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on)
 
 	if ((from + num <= from) || (from + num > IO_BITMAP_BITS))
 		return -EINVAL;
-	if (turn_on && !capable(CAP_SYS_RAWIO))
+	if (turn_on && (!capable(CAP_SYS_RAWIO) ||
+			kernel_is_locked_down("ioperm")))
 		return -EPERM;
 
 	/*
@@ -120,7 +121,8 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
 		return -EINVAL;
 	/* Trying to gain more privileges? */
 	if (level > old) {
-		if (!capable(CAP_SYS_RAWIO))
+		if (!capable(CAP_SYS_RAWIO) ||
+		    kernel_is_locked_down("iopl"))
 			return -EPERM;
 	}
 	regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) |
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index b7c36898b689..0875b3d47773 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -768,6 +768,8 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 
 static int open_port(struct inode *inode, struct file *filp)
 {
+	if (kernel_is_locked_down("Direct ioport access"))
+		return -EPERM;
 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
 }
 

Re: [PATCH 11/27] x86: Lock down IO port access when the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:51:56PM +0100, David Howells wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> IO port access would permit users to gain access to PCI configuration
> registers, which in turn (on a lot of hardware) give access to MMIO
> register space. This would potentially permit root to trigger arbitrary
> DMA, so lock it down by default.
> 
> This also implicitly locks down the KDADDIO, KDDELIO, KDENABIO and
> KDDISABIO console ioctls.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks!
Joey Lee

> cc: x86@kernel.org
> ---
> 
>  arch/x86/kernel/ioport.c |    6 ++++--
>  drivers/char/mem.c       |    2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
> index 9c3cf0944bce..2c0f058651c5 100644
> --- a/arch/x86/kernel/ioport.c
> +++ b/arch/x86/kernel/ioport.c
> @@ -30,7 +30,8 @@ asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int turn_on)
>  
>  	if ((from + num <= from) || (from + num > IO_BITMAP_BITS))
>  		return -EINVAL;
> -	if (turn_on && !capable(CAP_SYS_RAWIO))
> +	if (turn_on && (!capable(CAP_SYS_RAWIO) ||
> +			kernel_is_locked_down("ioperm")))
>  		return -EPERM;
>  
>  	/*
> @@ -120,7 +121,8 @@ SYSCALL_DEFINE1(iopl, unsigned int, level)
>  		return -EINVAL;
>  	/* Trying to gain more privileges? */
>  	if (level > old) {
> -		if (!capable(CAP_SYS_RAWIO))
> +		if (!capable(CAP_SYS_RAWIO) ||
> +		    kernel_is_locked_down("iopl"))
>  			return -EPERM;
>  	}
>  	regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) |
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index b7c36898b689..0875b3d47773 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -768,6 +768,8 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
>  
>  static int open_port(struct inode *inode, struct file *filp)
>  {
> +	if (kernel_is_locked_down("Direct ioport access"))
> +		return -EPERM;
>  	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
>  }
>  
> 
> --
> 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 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

Writing to MSRs should not be allowed if the kernel is locked down, since
it could lead to execution of arbitrary code in kernel mode.  Based on a
patch by Kees Cook.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
cc: x86@kernel.org
---

 arch/x86/kernel/msr.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index ef688804f80d..a05a97863286 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -84,6 +84,9 @@ static ssize_t msr_write(struct file *file, const char __user *buf,
 	int err = 0;
 	ssize_t bytes = 0;
 
+	if (kernel_is_locked_down("Direct MSR access"))
+		return -EPERM;
+
 	if (count % 8)
 		return -EINVAL;	/* Invalid chunk size */
 
@@ -131,6 +134,10 @@ static long msr_ioctl(struct file *file, unsigned int ioc, unsigned long arg)
 			err = -EBADF;
 			break;
 		}
+		if (kernel_is_locked_down("Direct MSR access")) {
+			err = -EPERM;
+			break;
+		}
 		if (copy_from_user(&regs, uregs, sizeof regs)) {
 			err = -EFAULT;
 			break;

Re: [PATCH 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:52:04PM +0100, David Howells wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> Writing to MSRs should not be allowed if the kernel is locked down, since
> it could lead to execution of arbitrary code in kernel mode.  Based on a
> patch by Kees Cook.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Acked-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks!
Joey Lee

> cc: x86@kernel.org
> ---
> 
>  arch/x86/kernel/msr.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
> index ef688804f80d..a05a97863286 100644
> --- a/arch/x86/kernel/msr.c
> +++ b/arch/x86/kernel/msr.c
> @@ -84,6 +84,9 @@ static ssize_t msr_write(struct file *file, const char __user *buf,
>  	int err = 0;
>  	ssize_t bytes = 0;
>  
> +	if (kernel_is_locked_down("Direct MSR access"))
> +		return -EPERM;
> +
>  	if (count % 8)
>  		return -EINVAL;	/* Invalid chunk size */
>  
> @@ -131,6 +134,10 @@ static long msr_ioctl(struct file *file, unsigned int ioc, unsigned long arg)
>  			err = -EBADF;
>  			break;
>  		}
> +		if (kernel_is_locked_down("Direct MSR access")) {
> +			err = -EPERM;
> +			break;
> +		}
>  		if (copy_from_user(&regs, uregs, sizeof regs)) {
>  			err = -EFAULT;
>  			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

Re: [PATCH 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[Alan Cox]

On Thu, 19 Oct 2017 15:52:04 +0100
David Howells <dhowells@redhat.com> wrote:

> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> Writing to MSRs should not be allowed if the kernel is locked down, since
> it could lead to execution of arbitrary code in kernel mode.  Based on a
> patch by Kees Cook.

There are a load of standard tools that use this so I think you are going
to need a whitelist. Can you at least log *which* MSR in the failing case
so a whitelist can be built over time ?

Alan

Re: [PATCH 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[David Howells]

Alan Cox <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote:

> There are a load of standard tools that use this so I think you are going
> to need a whitelist. Can you at least log *which* MSR in the failing case
> so a whitelist can be built over time ?

Probably.  Is it just the file position for msr_write()?  Should the register
number increment with the copy loop?

What about for the X86_IOC_WRMSR_REGS ioctl?  What's the reg number there?

And do you know where wrmsr_safe_regs() might be found?  I can see things
using it and exporting it, but no implementation, so I'm guessing it's
macroised somewhere.

David

Re: [PATCH 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[joeyli]

On Fri, Oct 20, 2017 at 09:48:16PM +0100, David Howells wrote:
> Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
> > There are a load of standard tools that use this so I think you are going
> > to need a whitelist. Can you at least log *which* MSR in the failing case
> > so a whitelist can be built over time ?
> 
[...snip]
> 
> And do you know where wrmsr_safe_regs() might be found?  I can see things
> using it and exporting it, but no implementation, so I'm guessing it's
> macroised somewhere.

Looks the definition is in 

arch/x86/lib/msr-reg.S

#ifdef CONFIG_X86_64
/*
 * int {rdmsr,wrmsr}_safe_regs(u32 gprs[8]);
 *
 * reg layout: u32 gprs[eax, ecx, edx, ebx, esp, ebp, esi, edi]
 *
 */
.macro op_safe_regs op
ENTRY(\op\()_safe_regs)
        pushq %rbx
        pushq %r12
...

Regards
Joey Lee

Re: [PATCH 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[David Howells]

Alan Cox <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote:

> There are a load of standard tools that use this so I think you are going
> to need a whitelist. Can you at least log *which* MSR in the failing case
> so a whitelist can be built over time ?

Will the attached change work for you?

David
---
diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index a05a97863286..f18cadbc31ce 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -84,8 +84,10 @@ static ssize_t msr_write(struct file *file, const char __user *buf,
 	int err = 0;
 	ssize_t bytes = 0;
 
-	if (kernel_is_locked_down("Direct MSR access"))
+	if (kernel_is_locked_down("Direct MSR access")) {
+		pr_info("Direct access to MSR %x\n", reg);
 		return -EPERM;
+	}
 
 	if (count % 8)
 		return -EINVAL;	/* Invalid chunk size */
@@ -135,6 +137,7 @@ static long msr_ioctl(struct file *file, unsigned int ioc, unsigned long arg)
 			break;
 		}
 		if (kernel_is_locked_down("Direct MSR access")) {
+			pr_info("Direct access to MSR %x\n", reg[1]); /* Display %ecx */
 			err = -EPERM;
 			break;
 		}

Re: [PATCH 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[joeyli]

Hi David, 

On Mon, Oct 23, 2017 at 03:49:44PM +0100, David Howells wrote:
> Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
> > There are a load of standard tools that use this so I think you are going
> > to need a whitelist. Can you at least log *which* MSR in the failing case
> > so a whitelist can be built over time ?
> 
> Will the attached change work for you?
> 

It's good to me.

Joey Lee

> ---
> diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
> index a05a97863286..f18cadbc31ce 100644
> --- a/arch/x86/kernel/msr.c
> +++ b/arch/x86/kernel/msr.c
> @@ -84,8 +84,10 @@ static ssize_t msr_write(struct file *file, const char __user *buf,
>  	int err = 0;
>  	ssize_t bytes = 0;
>  
> -	if (kernel_is_locked_down("Direct MSR access"))
> +	if (kernel_is_locked_down("Direct MSR access")) {
> +		pr_info("Direct access to MSR %x\n", reg);
>  		return -EPERM;
> +	}
>  
>  	if (count % 8)
>  		return -EINVAL;	/* Invalid chunk size */
> @@ -135,6 +137,7 @@ static long msr_ioctl(struct file *file, unsigned int ioc, unsigned long arg)
>  			break;
>  		}
>  		if (kernel_is_locked_down("Direct MSR access")) {
> +			pr_info("Direct access to MSR %x\n", reg[1]); /* Display %ecx */
>  			err = -EPERM;
>  			break;
>  		}

[PATCH 13/27] asus-wmi: Restrict debugfs interface when the kernel is locked down

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

We have no way of validating what all of the Asus WMI methods do on a given
machine - and there's a risk that some will allow hardware state to be
manipulated in such a way that arbitrary code can be executed in the
kernel, circumventing module loading restrictions.  Prevent that if the
kernel is locked down.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: acpi4asus-user@lists.sourceforge.net
cc: platform-driver-x86@vger.kernel.org
---

 drivers/platform/x86/asus-wmi.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 48e1541dc8d4..ef5587469337 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1905,6 +1905,9 @@ static int show_dsts(struct seq_file *m, void *data)
 	int err;
 	u32 retval = -1;
 
+	if (kernel_is_locked_down("Asus WMI"))
+		return -EPERM;
+
 	err = asus_wmi_get_devstate(asus, asus->debug.dev_id, &retval);
 
 	if (err < 0)
@@ -1921,6 +1924,9 @@ static int show_devs(struct seq_file *m, void *data)
 	int err;
 	u32 retval = -1;
 
+	if (kernel_is_locked_down("Asus WMI"))
+		return -EPERM;
+
 	err = asus_wmi_set_devstate(asus->debug.dev_id, asus->debug.ctrl_param,
 				    &retval);
 
@@ -1945,6 +1951,9 @@ static int show_call(struct seq_file *m, void *data)
 	union acpi_object *obj;
 	acpi_status status;
 
+	if (kernel_is_locked_down("Asus WMI"))
+		return -EPERM;
+
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
 				     0, asus->debug.method_id,
 				     &input, &output);

Re: [PATCH 13/27] asus-wmi: Restrict debugfs interface when the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:52:11PM +0100, David Howells wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> We have no way of validating what all of the Asus WMI methods do on a given
> machine - and there's a risk that some will allow hardware state to be
> manipulated in such a way that arbitrary code can be executed in the
> kernel, circumventing module loading restrictions.  Prevent that if the
> kernel is locked down.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks!
Joey Lee

> cc: acpi4asus-user@lists.sourceforge.net
> cc: platform-driver-x86@vger.kernel.org
> ---
> 
>  drivers/platform/x86/asus-wmi.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 48e1541dc8d4..ef5587469337 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1905,6 +1905,9 @@ static int show_dsts(struct seq_file *m, void *data)
>  	int err;
>  	u32 retval = -1;
>  
> +	if (kernel_is_locked_down("Asus WMI"))
> +		return -EPERM;
> +
>  	err = asus_wmi_get_devstate(asus, asus->debug.dev_id, &retval);
>  
>  	if (err < 0)
> @@ -1921,6 +1924,9 @@ static int show_devs(struct seq_file *m, void *data)
>  	int err;
>  	u32 retval = -1;
>  
> +	if (kernel_is_locked_down("Asus WMI"))
> +		return -EPERM;
> +
>  	err = asus_wmi_set_devstate(asus->debug.dev_id, asus->debug.ctrl_param,
>  				    &retval);
>  
> @@ -1945,6 +1951,9 @@ static int show_call(struct seq_file *m, void *data)
>  	union acpi_object *obj;
>  	acpi_status status;
>  
> +	if (kernel_is_locked_down("Asus WMI"))
> +		return -EPERM;
> +
>  	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
>  				     0, asus->debug.method_id,
>  				     &input, &output);
> 
> --
> 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 14/27] ACPI: Limit access to custom_method when the kernel is locked down

[David Howells]

From: Matthew Garrett <matthew.garrett@nebula.com>

custom_method effectively allows arbitrary access to system memory, making
it possible for an attacker to circumvent restrictions on module loading.
Disable it if the kernel is locked down.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-acpi@vger.kernel.org
---

 drivers/acpi/custom_method.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index c68e72414a67..b33fba70ec51 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -29,6 +29,9 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
 	struct acpi_table_header table;
 	acpi_status status;
 
+	if (kernel_is_locked_down("ACPI custom methods"))
+		return -EPERM;
+
 	if (!(*ppos)) {
 		/* parse the table header to get the table length */
 		if (count <= sizeof(struct acpi_table_header))

Re: [PATCH 14/27] ACPI: Limit access to custom_method when the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:52:19PM +0100, David Howells wrote:
> From: Matthew Garrett <matthew.garrett@nebula.com>
> 
> custom_method effectively allows arbitrary access to system memory, making
> it possible for an attacker to circumvent restrictions on module loading.
> Disable it if the kernel is locked down.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks!
Joey Lee

> cc: linux-acpi@vger.kernel.org
> ---
> 
>  drivers/acpi/custom_method.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
> index c68e72414a67..b33fba70ec51 100644
> --- a/drivers/acpi/custom_method.c
> +++ b/drivers/acpi/custom_method.c
> @@ -29,6 +29,9 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
>  	struct acpi_table_header table;
>  	acpi_status status;
>  
> +	if (kernel_is_locked_down("ACPI custom methods"))
> +		return -EPERM;
> +
>  	if (!(*ppos)) {
>  		/* parse the table header to get the table length */
>  		if (count <= sizeof(struct acpi_table_header))
> 
> --
> 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 15/27] acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down

[David Howells]

From: Josh Boyer <jwboyer@redhat.com>

This option allows userspace to pass the RSDP address to the kernel, which
makes it possible for a user to modify the workings of hardware .  Reject
the option when the kernel is locked down.

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Dave Young <dyoung@redhat.com>
cc: linux-acpi@vger.kernel.org
---

 drivers/acpi/osl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index db78d353bab1..36c6527c1b0a 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -192,7 +192,7 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 	acpi_physical_address pa = 0;
 
 #ifdef CONFIG_KEXEC
-	if (acpi_rsdp)
+	if (acpi_rsdp && !kernel_is_locked_down("ACPI RSDP specification"))
 		return acpi_rsdp;
 #endif
 

Re: [PATCH 15/27] acpi: Ignore acpi_rsdp kernel param when the kernel has been locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:52:27PM +0100, David Howells wrote:
> From: Josh Boyer <jwboyer@redhat.com>
> 
> This option allows userspace to pass the RSDP address to the kernel, which
> makes it possible for a user to modify the workings of hardware .  Reject
> the option when the kernel is locked down.
> 
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> Signed-off-by: David Howells <dhowells@redhat.com>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>

Thanks!
Joey Lee

> cc: Dave Young <dyoung@redhat.com>
> cc: linux-acpi@vger.kernel.org
> ---
> 
>  drivers/acpi/osl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index db78d353bab1..36c6527c1b0a 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -192,7 +192,7 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
>  	acpi_physical_address pa = 0;
>  
>  #ifdef CONFIG_KEXEC
> -	if (acpi_rsdp)
> +	if (acpi_rsdp && !kernel_is_locked_down("ACPI RSDP specification"))
>  		return acpi_rsdp;
>  #endif
>  
> 
> --
> 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 16/27] acpi: Disable ACPI table override if the kernel is locked down

[David Howells]

From: Linn Crosetto <linn@hpe.com>

>From the kernel documentation (initrd_table_override.txt):

  If the ACPI_INITRD_TABLE_OVERRIDE compile option is true, it is possible
  to override nearly any ACPI table provided by the BIOS with an
  instrumented, modified one.

When securelevel is set, the kernel should disallow any unauthenticated
changes to kernel space.  ACPI tables contain code invoked by the kernel,
so do not allow ACPI tables to be overridden if the kernel is locked down.

Signed-off-by: Linn Crosetto <linn@hpe.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-acpi@vger.kernel.org
---

 drivers/acpi/tables.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 80ce2a7d224b..5cc13c42daf9 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -526,6 +526,11 @@ void __init acpi_table_upgrade(void)
 	if (table_nr == 0)
 		return;
 
+	if (kernel_is_locked_down("ACPI table override")) {
+		pr_notice("kernel is locked down, ignoring table override\n");
+		return;
+	}
+
 	acpi_tables_addr =
 		memblock_find_in_range(0, ACPI_TABLE_UPGRADE_MAX_PHYS,
 				       all_tables_size, PAGE_SIZE);

Re: [PATCH 16/27] acpi: Disable ACPI table override if the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:52:34PM +0100, David Howells wrote:
> From: Linn Crosetto <linn-ZPxbGqLxI0U@public.gmane.org>
> 
> >From the kernel documentation (initrd_table_override.txt):
> 
>   If the ACPI_INITRD_TABLE_OVERRIDE compile option is true, it is possible
>   to override nearly any ACPI table provided by the BIOS with an
>   instrumented, modified one.
> 
> When securelevel is set, the kernel should disallow any unauthenticated
> changes to kernel space.  ACPI tables contain code invoked by the kernel,
> so do not allow ACPI tables to be overridden if the kernel is locked down.
> 
> Signed-off-by: Linn Crosetto <linn-ZPxbGqLxI0U@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>

Thanks!
Joey Lee

> cc: linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
> 
>  drivers/acpi/tables.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 80ce2a7d224b..5cc13c42daf9 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -526,6 +526,11 @@ void __init acpi_table_upgrade(void)
>  	if (table_nr == 0)
>  		return;
>  
> +	if (kernel_is_locked_down("ACPI table override")) {
> +		pr_notice("kernel is locked down, ignoring table override\n");
> +		return;
> +	}
> +
>  	acpi_tables_addr =
>  		memblock_find_in_range(0, ACPI_TABLE_UPGRADE_MAX_PHYS,
>  				       all_tables_size, PAGE_SIZE);
> 
> --
> 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 17/27] acpi: Disable APEI error injection if the kernel is locked down

[David Howells]

From: Linn Crosetto <linn@hpe.com>

ACPI provides an error injection mechanism, EINJ, for debugging and testing
the ACPI Platform Error Interface (APEI) and other RAS features.  If
supported by the firmware, ACPI specification 5.0 and later provide for a
way to specify a physical memory address to which to inject the error.

Injecting errors through EINJ can produce errors which to the platform are
indistinguishable from real hardware errors.  This can have undesirable
side-effects, such as causing the platform to mark hardware as needing
replacement.

While it does not provide a method to load unauthenticated privileged code,
the effect of these errors may persist across reboots and affect trust in
the underlying hardware, so disable error injection through EINJ if
the kernel is locked down.

Signed-off-by: Linn Crosetto <linn@hpe.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-acpi@vger.kernel.org
---

 drivers/acpi/apei/einj.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index b38737c83a24..6d71e1e97b20 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -518,6 +518,9 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
 	int rc;
 	u64 base_addr, size;
 
+	if (kernel_is_locked_down("ACPI error injection"))
+		return -EPERM;
+
 	/* If user manually set "flags", make sure it is legal */
 	if (flags && (flags &
 		~(SETWA_FLAGS_APICID|SETWA_FLAGS_MEM|SETWA_FLAGS_PCIE_SBDF)))

Re: [PATCH 17/27] acpi: Disable APEI error injection if the kernel is locked down

[joeyli]

On Thu, Oct 19, 2017 at 03:52:41PM +0100, David Howells wrote:
> From: Linn Crosetto <linn-ZPxbGqLxI0U@public.gmane.org>
> 
> ACPI provides an error injection mechanism, EINJ, for debugging and testing
> the ACPI Platform Error Interface (APEI) and other RAS features.  If
> supported by the firmware, ACPI specification 5.0 and later provide for a
> way to specify a physical memory address to which to inject the error.
> 
> Injecting errors through EINJ can produce errors which to the platform are
> indistinguishable from real hardware errors.  This can have undesirable
> side-effects, such as causing the platform to mark hardware as needing
> replacement.
> 
> While it does not provide a method to load unauthenticated privileged code,
> the effect of these errors may persist across reboots and affect trust in
> the underlying hardware, so disable error injection through EINJ if
> the kernel is locked down.
> 
> Signed-off-by: Linn Crosetto <linn-ZPxbGqLxI0U@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I have reviewed this patch. Please feel free to add:

Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>

Thanks!
Joey Lee

> cc: linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
> 
>  drivers/acpi/apei/einj.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index b38737c83a24..6d71e1e97b20 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -518,6 +518,9 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>  	int rc;
>  	u64 base_addr, size;
>  
> +	if (kernel_is_locked_down("ACPI error injection"))
> +		return -EPERM;
> +
>  	/* If user manually set "flags", make sure it is legal */
>  	if (flags && (flags &
>  		~(SETWA_FLAGS_APICID|SETWA_FLAGS_MEM|SETWA_FLAGS_PCIE_SBDF)))
> 
> --
> 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 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

From: Chun-Yi Lee <jlee@suse.com>

There are some bpf functions can be used to read kernel memory:
bpf_probe_read, bpf_probe_write_user and bpf_trace_printk.  These allow
private keys in kernel memory (e.g. the hibernation image signing key) to
be read by an eBPF program.  Prohibit those functions when the kernel is
locked down.

Signed-off-by: Chun-Yi Lee <jlee@suse.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: netdev@vger.kernel.org
---

 kernel/trace/bpf_trace.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index dc498b605d5d..35e85a3fdb37 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
 {
 	int ret;
 
+	if (kernel_is_locked_down("BPF")) {
+		memset(dst, 0, size);
+		return -EPERM;
+	}
+
 	ret = probe_kernel_read(dst, unsafe_ptr, size);
 	if (unlikely(ret < 0))
 		memset(dst, 0, size);
@@ -84,6 +89,9 @@ static const struct bpf_func_proto bpf_probe_read_proto = {
 BPF_CALL_3(bpf_probe_write_user, void *, unsafe_ptr, const void *, src,
 	   u32, size)
 {
+	if (kernel_is_locked_down("BPF"))
+		return -EPERM;
+
 	/*
 	 * Ensure we're in user context which is safe for the helper to
 	 * run. This helper has no business in a kthread.
@@ -143,6 +151,9 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
 	if (fmt[--fmt_size] != 0)
 		return -EINVAL;
 
+	if (kernel_is_locked_down("BPF"))
+		return __trace_printk(1, fmt, 0, 0, 0);
+
 	/* check format string for allowed specifiers */
 	for (i = 0; i < fmt_size; i++) {
 		if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i]))

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[Alexei Starovoitov]

On Thu, Oct 19, 2017 at 03:52:49PM +0100, David Howells wrote:
> From: Chun-Yi Lee <jlee@suse.com>
> 
> There are some bpf functions can be used to read kernel memory:
> bpf_probe_read, bpf_probe_write_user and bpf_trace_printk.  These allow
> private keys in kernel memory (e.g. the hibernation image signing key) to
> be read by an eBPF program.  Prohibit those functions when the kernel is
> locked down.
> 
> Signed-off-by: Chun-Yi Lee <jlee@suse.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: netdev@vger.kernel.org
> ---
> 
>  kernel/trace/bpf_trace.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index dc498b605d5d..35e85a3fdb37 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
>  {
>  	int ret;
>  
> +	if (kernel_is_locked_down("BPF")) {
> +		memset(dst, 0, size);
> +		return -EPERM;
> +	}

That doesn't help the lockdown purpose.
If you don't trust the root the only way to prevent bpf read
memory is to disable the whole thing.
Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
and don't add overhead to critical path like bpf_probe_read().

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[joeyli]

Hi Alexei,

Thanks for your review! 

On Thu, Oct 19, 2017 at 03:18:30PM -0700, Alexei Starovoitov wrote:
> On Thu, Oct 19, 2017 at 03:52:49PM +0100, David Howells wrote:
> > From: Chun-Yi Lee <jlee@suse.com>
> > 
> > There are some bpf functions can be used to read kernel memory:
> > bpf_probe_read, bpf_probe_write_user and bpf_trace_printk.  These allow
> > private keys in kernel memory (e.g. the hibernation image signing key) to
> > be read by an eBPF program.  Prohibit those functions when the kernel is
> > locked down.
> > 
> > Signed-off-by: Chun-Yi Lee <jlee@suse.com>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > cc: netdev@vger.kernel.org
> > ---
> > 
> >  kernel/trace/bpf_trace.c |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index dc498b605d5d..35e85a3fdb37 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
> >  {
> >  	int ret;
> >  
> > +	if (kernel_is_locked_down("BPF")) {
> > +		memset(dst, 0, size);
> > +		return -EPERM;
> > +	}
> 
> That doesn't help the lockdown purpose.
> If you don't trust the root the only way to prevent bpf read
> memory is to disable the whole thing.

Not totally untrust root, I don't want that root reads
arbitrary memory address through bpf.

Is it not enough to lock down bpf_probe_read, bpf_probe_write_user
and bpf_trace_printk?

> Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
> and don't add overhead to critical path like bpf_probe_read().
>

Yes, it give overhead to bpf_probe_read but it prevents arbitrary
memory read.

Another idea is signing bpf bytecode then verifying signture when
loading to kernel. 

Thanks a lot!
Joey Lee 

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

Hi Joey,

Should I just lock down sys_bpf() entirely for now?  We can always free it up
somewhat later.

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[jlee]

On Fri, Oct 20, 2017 at 09:08:48AM +0100, David Howells wrote:
> Hi Joey,
> 
> Should I just lock down sys_bpf() entirely for now?  We can always free it up
> somewhat later.
> 
> David

OK~~ Please just remove my patch until we find out a way to
verify bpf code or protect sensitive data in memory.

I think that we don't need to lock down sys_bpf() now because
we didn't lock down other interfaces for reading arbitrary
address like /dev/mem and /dev/kmem.

Thanks a lot!
Joey Lee

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

jlee-IBi9RG/b67k@public.gmane.org wrote:

> I think that we don't need to lock down sys_bpf() now because
> we didn't lock down other interfaces for reading arbitrary
> address like /dev/mem and /dev/kmem.

Ummm...  See patch 4.  You even gave me a Reviewed-by for it ;-)

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[jlee]

On Fri, Oct 20, 2017 at 05:03:22PM +0100, David Howells wrote:
> jlee@suse.com wrote:
> 
> > I think that we don't need to lock down sys_bpf() now because
> > we didn't lock down other interfaces for reading arbitrary
> > address like /dev/mem and /dev/kmem.
> 
> Ummm...  See patch 4.  You even gave me a Reviewed-by for it ;-)
> 
> David

hm... patch 4 only prevents write_mem() but not read_mem().
Or I missed anything?

Thanks
Joey Lee

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

jlee@suse.com wrote:

> hm... patch 4 only prevents write_mem() but not read_mem().
> Or I missed anything?

Actually, yes, as it happens, patch 11 prevents you from even opening /dev/mem
and /dev/kmem by locking down open of /dev/port.  So I've moved this bit to
patch 4, simplified and posted a new variant for patch 4.

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[joeyli]

On Mon, Oct 23, 2017 at 03:53:00PM +0100, David Howells wrote:
> jlee-IBi9RG/b67k@public.gmane.org wrote:
> 
> > hm... patch 4 only prevents write_mem() but not read_mem().
> > Or I missed anything?
> 
> Actually, yes, as it happens, patch 11 prevents you from even opening /dev/mem
> and /dev/kmem by locking down open of /dev/port.  So I've moved this bit to
> patch 4, simplified and posted a new variant for patch 4.
>

Thank you for pointing out!

Joey Lee

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > @@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
> >  {
> >  	int ret;
> >  
> > +	if (kernel_is_locked_down("BPF")) {
> > +		memset(dst, 0, size);
> > +		return -EPERM;
> > +	}
>
> That doesn't help the lockdown purpose.
> If you don't trust the root the only way to prevent bpf read
> memory is to disable the whole thing.
> Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
> and don't add overhead to critical path like bpf_probe_read().

TBH, I've no idea how bpf does anything, so I can't say whether this is
better, overkill or insufficient.

David

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[Alexei Starovoitov]

On Thu, Oct 19, 2017 at 11:48:34PM +0100, David Howells wrote:
> Alexei Starovoitov <alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > > @@ -65,6 +65,11 @@ BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
> > >  {
> > >  	int ret;
> > >  
> > > +	if (kernel_is_locked_down("BPF")) {
> > > +		memset(dst, 0, size);
> > > +		return -EPERM;
> > > +	}
> >
> > That doesn't help the lockdown purpose.
> > If you don't trust the root the only way to prevent bpf read
> > memory is to disable the whole thing.
> > Have a single check in sys_bpf() to disallow everything if kernel_is_locked_down()
> > and don't add overhead to critical path like bpf_probe_read().
> 
> TBH, I've no idea how bpf does anything, so I can't say whether this is
> better, overkill or insufficient.

ok. To make it clear:
Nacked-by: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
For the current patch.
Unnecessary checks for no good reason in performance critical
functions are not acceptable.

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > TBH, I've no idea how bpf does anything, so I can't say whether this is
> > better, overkill or insufficient.
> 
> ok. To make it clear:
> Nacked-by: Alexei Starovoitov <ast@kernel.org>
> For the current patch.
> Unnecessary checks for no good reason in performance critical
> functions are not acceptable.

They aren't unnecessary checks.

Can you please suggest if there's some way more suitable than just killing bpf
entirely?  I don't know the code, and I presume you do.

David

[PATCH 19/27] scsi: Lock down the eata driver

[David Howells]

When the kernel is running in secure boot mode, we lock down the kernel to
prevent userspace from modifying the running kernel image.  Whilst this
includes prohibiting access to things like /dev/mem, it must also prevent
access by means of configuring driver modules in such a way as to cause a
device to access or modify the kernel image.

The eata driver takes a single string parameter that contains a slew of
settings, including hardware resource configuration.  Prohibit use of the
parameter if the kernel is locked down.

Suggested-by: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Dario Ballabio <ballabio_dario@emc.com>
cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
cc: "Martin K. Petersen" <martin.petersen@oracle.com>
cc: linux-scsi@vger.kernel.org
---

 drivers/scsi/eata.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index 6501c330d8c8..72fceaa8f3da 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -1552,8 +1552,11 @@ static int eata2x_detect(struct scsi_host_template *tpnt)
 
 	tpnt->proc_name = "eata2x";
 
-	if (strlen(boot_options))
+	if (strlen(boot_options)) {
+		if (kernel_is_locked_down("Command line-specified device addresses, irqs and dma channels"))
+			return -EPERM;
 		option_setup(boot_options);
+	}
 
 #if defined(MODULE)
 	/* io_port could have been modified when loading as a module */

[PATCH 20/27] Prohibit PCMCIA CIS storage when the kernel is locked down

[David Howells]

Prohibit replacement of the PCMCIA Card Information Structure when the
kernel is locked down.

Suggested-by: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-pcmcia@lists.infradead.org
---

 drivers/pcmcia/cistpl.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pcmcia/cistpl.c b/drivers/pcmcia/cistpl.c
index 55ef7d1fd8da..b7a0e42eeb25 100644
--- a/drivers/pcmcia/cistpl.c
+++ b/drivers/pcmcia/cistpl.c
@@ -1578,6 +1578,9 @@ static ssize_t pccard_store_cis(struct file *filp, struct kobject *kobj,
 	struct pcmcia_socket *s;
 	int error;
 
+	if (kernel_is_locked_down("Direct PCMCIA CIS storage"))
+		return -EPERM;
+
 	s = to_socket(container_of(kobj, struct device, kobj));
 
 	if (off)

[PATCH 21/27] Lock down TIOCSSERIAL

[David Howells]

Lock down TIOCSSERIAL as that can be used to change the ioport and irq
settings on a serial port.  This only appears to be an issue for the serial
drivers that use the core serial code.  All other drivers seem to either
ignore attempts to change port/irq or give an error.

Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jiri Slaby <jslaby@suse.com>
---

 drivers/tty/serial/serial_core.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 3a14cccbd7ff..41f0922ad842 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -842,6 +842,12 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	new_flags = (__force upf_t)new_info->flags;
 	old_custom_divisor = uport->custom_divisor;
 
+	if ((change_port || change_irq) &&
+	    kernel_is_locked_down("Using TIOCSSERIAL to change device addresses, irqs and dma channels")) {
+		retval = -EPERM;
+		goto exit;
+	}
+
 	if (!capable(CAP_SYS_ADMIN)) {
 		retval = -EPERM;
 		if (change_irq || change_port ||

[PATCH 22/27] Lock down module params that specify hardware parameters (eg. ioport)

[David Howells]

Provided an annotation for module parameters that specify hardware
parameters (such as io ports, iomem addresses, irqs, dma channels, fixed
dma buffers and other types).

Suggested-by: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 kernel/params.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 60b2d8101355..422979adb60a 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -108,13 +108,19 @@ bool parameq(const char *a, const char *b)
 	return parameqn(a, b, strlen(a)+1);
 }
 
-static void param_check_unsafe(const struct kernel_param *kp)
+static bool param_check_unsafe(const struct kernel_param *kp,
+			       const char *doing)
 {
 	if (kp->flags & KERNEL_PARAM_FL_UNSAFE) {
 		pr_warn("Setting dangerous option %s - tainting kernel\n",
 			kp->name);
 		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
 	}
+
+	if (kp->flags & KERNEL_PARAM_FL_HWPARAM &&
+	    kernel_is_locked_down("Command line-specified device addresses, irqs and dma channels"))
+		return false;
+	return true;
 }
 
 static int parse_one(char *param,
@@ -144,8 +150,10 @@ static int parse_one(char *param,
 			pr_debug("handling %s with %p\n", param,
 				params[i].ops->set);
 			kernel_param_lock(params[i].mod);
-			param_check_unsafe(&params[i]);
-			err = params[i].ops->set(val, &params[i]);
+			if (param_check_unsafe(&params[i], doing))
+				err = params[i].ops->set(val, &params[i]);
+			else
+				err = -EPERM;
 			kernel_param_unlock(params[i].mod);
 			return err;
 		}
@@ -556,6 +564,12 @@ static ssize_t param_attr_show(struct module_attribute *mattr,
 	return count;
 }
 
+#ifdef CONFIG_MODULES
+#define mod_name(mod) (mod)->name
+#else
+#define mod_name(mod) "unknown"
+#endif
+
 /* sysfs always hands a nul-terminated string in buf.  We rely on that. */
 static ssize_t param_attr_store(struct module_attribute *mattr,
 				struct module_kobject *mk,
@@ -568,8 +582,10 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
 		return -EPERM;
 
 	kernel_param_lock(mk->mod);
-	param_check_unsafe(attribute->param);
-	err = attribute->param->ops->set(buf, attribute->param);
+	if (param_check_unsafe(attribute->param, mod_name(mk->mod)))
+		err = attribute->param->ops->set(buf, attribute->param);
+	else
+		err = -EPERM;
 	kernel_param_unlock(mk->mod);
 	if (!err)
 		return len;

[PATCH 23/27] x86/mmiotrace: Lock down the testmmiotrace module

[David Howells]

The testmmiotrace module shouldn't be permitted when the kernel is locked
down as it can be used to arbitrarily read and write MMIO space.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: David Howells <dhowells@redhat.com
cc: Thomas Gleixner <tglx@linutronix.de>
cc: Steven Rostedt <rostedt@goodmis.org>
cc: Ingo Molnar <mingo@kernel.org>
cc: "H. Peter Anvin" <hpa@zytor.com>
cc: x86@kernel.org
---

 arch/x86/mm/testmmiotrace.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/mm/testmmiotrace.c b/arch/x86/mm/testmmiotrace.c
index f6ae6830b341..bbaad357f5d7 100644
--- a/arch/x86/mm/testmmiotrace.c
+++ b/arch/x86/mm/testmmiotrace.c
@@ -115,6 +115,9 @@ static int __init init(void)
 {
 	unsigned long size = (read_far) ? (8 << 20) : (16 << 10);
 
+	if (kernel_is_locked_down("MMIO trace testing"))
+		return -EPERM;
+
 	if (mmio_address == 0) {
 		pr_err("you have to use the module argument mmio_address.\n");
 		pr_err("DO NOT LOAD THIS MODULE UNLESS YOU REALLY KNOW WHAT YOU ARE DOING!\n");

[PATCH 24/27] debugfs: Disallow use of debugfs files when the kernel is locked down

[David Howells]

Disallow opening of debugfs files when the kernel is locked down as various
drivers give raw access to hardware through debugfs.

Accesses to tracefs should use /sys/kernel/tracing/ rather than
/sys/kernel/debug/tracing/.  Possibly a symlink should be emplaced.

Normal device interaction should be done through configfs or a miscdev, not
debugfs.

Note that this makes it unnecessary to specifically lock down show_dsts(),
show_devs() and show_call() in the asus-wmi driver.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Andy Shevchenko <andy.shevchenko@gmail.com>
cc: acpi4asus-user@lists.sourceforge.net
cc: platform-driver-x86@vger.kernel.org
cc: Matthew Garrett <matthew.garrett@nebula.com>
cc: Thomas Gleixner <tglx@linutronix.de>
---

 fs/debugfs/file.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 6dabc4a10396..32b5168a7e91 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -103,6 +103,9 @@ static int open_proxy_open(struct inode *inode, struct file *filp)
 	const struct file_operations *real_fops = NULL;
 	int srcu_idx, r;
 
+	if (kernel_is_locked_down("debugfs"))
+		return -EPERM;
+
 	r = debugfs_use_file_start(dentry, &srcu_idx);
 	if (r) {
 		r = -ENOENT;
@@ -232,6 +235,9 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
 	struct file_operations *proxy_fops = NULL;
 	int srcu_idx, r;
 
+	if (kernel_is_locked_down("debugfs"))
+		return -EPERM;
+
 	r = debugfs_use_file_start(dentry, &srcu_idx);
 	if (r) {
 		r = -ENOENT;

[PATCH 25/27] Lock down /proc/kcore

[David Howells]

Disallow access to /proc/kcore when the kernel is locked down to prevent
access to cryptographic data.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/proc/kcore.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 45629f4b5402..176cf749e650 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -549,6 +549,8 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)
 
 static int open_kcore(struct inode *inode, struct file *filp)
 {
+	if (kernel_is_locked_down("/proc/kcore"))
+		return -EPERM;
 	if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
 

Re: [PATCH 25/27] Lock down /proc/kcore

[James Morris]

On Thu, 19 Oct 2017, David Howells wrote:

> Disallow access to /proc/kcore when the kernel is locked down to prevent
> access to cryptographic data.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Reviewed-by: James Morris <james.l.morris@oracle.com>

I have to wonder, though, after everything is locked down, how easy will 
it be for new things to slip in which need to be included in the lockdown, 
but are not.


-- 
James Morris
<james.l.morris@oracle.com>

Re: [PATCH 25/27] Lock down /proc/kcore

[David Howells]

James Morris <james.l.morris@oracle.com> wrote:

> I have to wonder, though, after everything is locked down, how easy will 
> it be for new things to slip in which need to be included in the lockdown, 
> but are not.

That's always a possibility, and short of reviewing every change, particularly
in the drivers, I'm not sure how to prevent it.

David

[PATCH 26/27] efi: Add an EFI_SECURE_BOOT flag to indicate secure boot mode

[David Howells]

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

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

Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
cc: linux-efi@vger.kernel.org
---

 arch/x86/kernel/setup.c           |   14 +-------------
 drivers/firmware/efi/Makefile     |    1 +
 drivers/firmware/efi/secureboot.c |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/efi.h               |   16 ++++++++++------
 4 files changed, 49 insertions(+), 19 deletions(-)
 create mode 100644 drivers/firmware/efi/secureboot.c

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0957dd73d127..7c2162f9e769 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1197,19 +1197,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/Makefile b/drivers/firmware/efi/Makefile
index 0329d319d89a..883f9f7eefc6 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)			+= 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..674dcc01bb0b
--- /dev/null
+++ b/drivers/firmware/efi/secureboot.c
@@ -0,0 +1,37 @@
+/* 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.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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:
+			set_bit(EFI_SECURE_BOOT, &efi.flags);
+			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 66f4a4e79f4b..7c7a7e33e4d1 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1103,6 +1103,14 @@ 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? */
+
+enum efi_secureboot_mode {
+	efi_secureboot_mode_unset,
+	efi_secureboot_mode_unknown,
+	efi_secureboot_mode_disabled,
+	efi_secureboot_mode_enabled,
+};
 
 #ifdef CONFIG_EFI
 /*
@@ -1115,6 +1123,7 @@ static inline bool efi_enabled(int feature)
 extern void efi_reboot(enum reboot_mode reboot_mode, const char *__unused);
 
 extern bool efi_is_table_address(unsigned long phys_addr);
+extern void __init efi_set_secure_boot(enum efi_secureboot_mode mode);
 #else
 static inline bool efi_enabled(int feature)
 {
@@ -1133,6 +1142,7 @@ static inline bool efi_is_table_address(unsigned long phys_addr)
 {
 	return false;
 }
+static inline void efi_set_secure_boot(enum efi_secureboot_mode mode) {}
 #endif
 
 extern int efi_status_to_err(efi_status_t status);
@@ -1518,12 +1528,6 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
 bool efi_runtime_disabled(void);
 extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
 
-enum efi_secureboot_mode {
-	efi_secureboot_mode_unset,
-	efi_secureboot_mode_unknown,
-	efi_secureboot_mode_disabled,
-	efi_secureboot_mode_enabled,
-};
 enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
 
 #ifdef CONFIG_RESET_ATTACK_MITIGATION

Re: [PATCH 26/27] efi: Add an EFI_SECURE_BOOT flag to indicate secure boot mode

[James Morris]

On Thu, 19 Oct 2017, David Howells wrote:

> +	if (efi_enabled(EFI_BOOT)) {
> +		switch (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");
> +			break;
> +		default:
> +			pr_info("Secure boot could not be determined\n");

Perhaps make this pr_warning and include the unknown mode value?

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

Re: [PATCH 26/27] efi: Add an EFI_SECURE_BOOT flag to indicate secure boot mode

[David Howells]

James Morris <james.l.morris@oracle.com> wrote:

> > +		default:
> > +			pr_info("Secure boot could not be determined\n");
> 
> Perhaps make this pr_warning and include the unknown mode value?

Done.

David

[PATCH 27/27] 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>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
cc: linux-efi@vger.kernel.org
---

 arch/x86/kernel/setup.c |    6 ++++--
 security/Kconfig        |   14 ++++++++++++++
 security/lock_down.c    |    1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 7c2162f9e769..4e38327efb2e 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -64,6 +64,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/ctype.h>
 #include <linux/uaccess.h>
+#include <linux/security.h>
 
 #include <linux/percpu.h>
 #include <linux/crash_dump.h>
@@ -1039,6 +1040,9 @@ void __init setup_arch(char **cmdline_p)
 	if (efi_enabled(EFI_BOOT))
 		efi_init();
 
+	efi_set_secure_boot(boot_params.secure_boot);
+	init_lockdown();
+
 	dmi_scan_machine();
 	dmi_memdev_walk();
 	dmi_set_dump_stack_arch_desc();
@@ -1197,8 +1201,6 @@ void __init setup_arch(char **cmdline_p)
 	/* Allocate bigger log buffer */
 	setup_log_buf(1);
 
-	efi_set_secure_boot(boot_params.secure_boot);
-
 	reserve_initrd();
 
 	acpi_table_upgrade();
diff --git a/security/Kconfig b/security/Kconfig
index 4be6be71e075..e1756039dc0a 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -227,6 +227,20 @@ config ALLOW_LOCKDOWN_LIFT_BY_SYSRQ
 	  Allow the lockdown on a kernel to be lifted, by pressing a SysRq key
 	  combination on a wired keyboard.
 
+config LOCK_DOWN_IN_EFI_SECURE_BOOT
+	bool "Lock down the kernel in EFI Secure Boot mode"
+	default n
+	select LOCK_DOWN_KERNEL
+	depends on EFI
+	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 results in kernel lockdown being
+	  triggered if EFI Secure Boot is set.
+
 
 source security/selinux/Kconfig
 source security/smack/Kconfig
diff --git a/security/lock_down.c b/security/lock_down.c
index f71118c340d2..12c3bc204c4e 100644
--- a/security/lock_down.c
+++ b/security/lock_down.c
@@ -12,6 +12,7 @@
 #include <linux/security.h>
 #include <linux/export.h>
 #include <linux/sysrq.h>
+#include <linux/efi.h>
 
 #ifdef CONFIG_ALLOW_LOCKDOWN_LIFT
 static __read_mostly bool kernel_locked_down;

Re: [PATCH 00/27] security, efi: Add kernel lockdown

[David Howells]

I've pushed a new version to git that fixes bugs in patches 1 and 2.

David

Re: [PATCH 04/27] Restrict /dev/mem and /dev/kmem when the kernel is locked down

[David Howells]

I think I should replace this patch with the attached.  This will prevent
/dev/mem, /dev/kmem and /dev/port from being *opened*, and thereby preventing
read, write and ioctl.

David
---
commit e68daa2256986932b9a7d6709cf9e24b30d93583
Author: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
Date:   Wed May 24 14:56:02 2017 +0100

    Restrict /dev/{mem,kmem,port} when the kernel is locked down
    
    Allowing users to read and write to core kernel memory makes it possible
    for the kernel to be subverted, avoiding module loading restrictions, and
    also to steal cryptographic information.
    
    Disallow /dev/mem and /dev/kmem from being opened this when the kernel has
    been locked down to prevent this.
    
    Also disallow /dev/port from being opened to prevent raw ioport access and
    thus DMA from being used to accomplish the same thing.
    
    Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
    Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 593a8818aca9..0ce5ac0a5c6b 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -762,6 +762,8 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 
 static int open_port(struct inode *inode, struct file *filp)
 {
+	if (kernel_is_locked_down("/dev/mem,kmem,port"))
+		return -EPERM;
 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
 }
 

Re: [PATCH 04/27] Restrict /dev/mem and /dev/kmem when the kernel is locked down

[Ethan Zhao]

David，

    May I ask a question here -- Is it intentionally enabling the
read-only mode, so userspace
tools like dmidecode could work with kernel_is_locked_down ?  while it
was impossible to work
with the attached patch applied. Is it a security policy change with
secure boot ?

Thanks,
Ethan

On Mon, Oct 23, 2017 at 10:34 PM, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> I think I should replace this patch with the attached.  This will prevent
> /dev/mem, /dev/kmem and /dev/port from being *opened*, and thereby preventing
> read, write and ioctl.
>
> David
> ---
> commit e68daa2256986932b9a7d6709cf9e24b30d93583
> Author: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
> Date:   Wed May 24 14:56:02 2017 +0100
>
>     Restrict /dev/{mem,kmem,port} when the kernel is locked down
>
>     Allowing users to read and write to core kernel memory makes it possible
>     for the kernel to be subverted, avoiding module loading restrictions, and
>     also to steal cryptographic information.
>
>     Disallow /dev/mem and /dev/kmem from being opened this when the kernel has
>     been locked down to prevent this.
>
>     Also disallow /dev/port from being opened to prevent raw ioport access and
>     thus DMA from being used to accomplish the same thing.
>
>     Signed-off-by: Matthew Garrett <matthew.garrett-05XSO3Yj/JvQT0dZR+AlfA@public.gmane.org>
>     Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     Reviewed-by: "Lee, Chun-Yi" <jlee-IBi9RG/b67k@public.gmane.org>
>
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 593a8818aca9..0ce5ac0a5c6b 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -762,6 +762,8 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
>
>  static int open_port(struct inode *inode, struct file *filp)
>  {
> +       if (kernel_is_locked_down("/dev/mem,kmem,port"))
> +               return -EPERM;
>         return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
>  }
>

Re: [PATCH 04/27] Restrict /dev/mem and /dev/kmem when the kernel is locked down

[David Howells]

Ethan Zhao <ethan.kernel@gmail.com> wrote:

>     May I ask a question here -- Is it intentionally enabling the
> read-only mode, so userspace
> tools like dmidecode could work with kernel_is_locked_down ?  while it
> was impossible to work
> with the attached patch applied. Is it a security policy change with
> secure boot ?

I removed readability on /dev/mem, /dev/kmem and /proc/kcore so that userspace
can't use this to gain access to cryptographic material in use by the kernel.

Readability was removed on /dev/port because reading from an I/O port register
might have a side effect or might allow you to snoop h/w interactions, such as
keyboard input.

I can provide an additional config option to allow /dev/mem and similar to
remain readable - but it needs to be a temporary affair.

I can also log accesses to these interfaces so that we can find out what
breaks and fix it.

Note that dmidecode doesn't necessarily use /dev/mem:

	[root@andromeda ~]# strace -f -eopen dmidecode  >/dev/null
	open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
	open("/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
	open("/sys/firmware/dmi/tables/smbios_entry_point", O_RDONLY) = 3
	open("/sys/firmware/dmi/tables/DMI", O_RDONLY) = 3
	+++ exited with 0 +++

Indeed, my Fedora 24 test system boots without a /dev/mem file being present
(I'm not sure *why* /dev/mem isn't present, but I hadn't noticed till now).

David

Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

Hi David,

>From the man page:

> Only validly signed modules may be loaded.
> .P
> Only validly signed binaries may be kexec'd.
> .P
> Only validly signed device firmware may be loaded.

fw_get_filesystem_firmware() calls kernel_read_file_from_path() to
read the firmware, which calls into the security hooks. Is there
another place that validates the firmware signatures.  I'm not seeing
which patch requires firmware to be signed?

Mimi

Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[David Howells]

Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:

> > Only validly signed device firmware may be loaded.
> 
> fw_get_filesystem_firmware() calls kernel_read_file_from_path() to
> read the firmware, which calls into the security hooks. Is there
> another place that validates the firmware signatures.  I'm not seeing
> which patch requires firmware to be signed?

Luis has a set of patches for this.  However, I'm not sure if that's going
anywhere at the moment.  Possibly I should remove this from the manpage for
the moment.

David

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Thu, 2017-11-02 at 22:04 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > > Only validly signed device firmware may be loaded.
> > 
> > fw_get_filesystem_firmware() calls kernel_read_file_from_path() to
> > read the firmware, which calls into the security hooks. Is there
> > another place that validates the firmware signatures.  I'm not seeing
> > which patch requires firmware to be signed?
> 
> Luis has a set of patches for this.  However, I'm not sure if that's going
> anywhere at the moment.  Possibly I should remove this from the manpage for
> the moment.

Or reflect that IMA-appraisal, if enabled, will enforce firmware being
validly signed.

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Thu, Nov 02, 2017 at 06:10:41PM -0400, Mimi Zohar wrote:
> On Thu, 2017-11-02 at 22:04 +0000, David Howells wrote:
> > Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> > 
> > > > Only validly signed device firmware may be loaded.
> > > 
> > > fw_get_filesystem_firmware() calls kernel_read_file_from_path() to
> > > read the firmware, which calls into the security hooks. Is there
> > > another place that validates the firmware signatures.  I'm not seeing
> > > which patch requires firmware to be signed?
> > 
> > Luis has a set of patches for this.  However, I'm not sure if that's going
> > anywhere at the moment.  Possibly I should remove this from the manpage for
> > the moment.

Remove it for now. The state of of affairs for firmware signing is complex given
that we first wanted to address how to properly grow the API without making
the API worse. This in and of itself was an effort, and that effort also
evaluated two different development paradigms:

	o functional API
	o data driven API

I only recently was convinced that functional API should be used, even for
commonly used exported symbols, and as such I've been going back and slowly
grooming the firmware API with small atomic changes to first clean up the
complex flag mess we have.

Since I'm busy with that Takahiro AKASHI has taken up firmware singing effort
but this will depend on the above small cleanup to be done first. I was busy
with addressing existing bugs on the firmware API for a while, then company
travel / conferences so was not able to address this, but I'm back now and
I believe I should be able to tackle the cleanup now.

Only after this is merged can we expect a final respin of the firmware signing
effort.

> Or reflect that IMA-appraisal, if enabled, will enforce firmware being
> validly signed.

But FWICT lockdown is a built-in kernel thingy, unless lockdown implies IMA
it would not be the place to refer to it.

It seems the documentation was proposed to help users if an error was caught.
That error should cover only what is being addressed in code on the kernel.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[AKASHI, Takahiro]

Luis,

Thank you for this heads-up.

On Wed, Nov 08, 2017 at 12:07:00AM +0100, Luis R. Rodriguez wrote:
> On Thu, Nov 02, 2017 at 06:10:41PM -0400, Mimi Zohar wrote:
> > On Thu, 2017-11-02 at 22:04 +0000, David Howells wrote:
> > > Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> > > 
> > > > > Only validly signed device firmware may be loaded.
> > > > 
> > > > fw_get_filesystem_firmware() calls kernel_read_file_from_path() to
> > > > read the firmware, which calls into the security hooks. Is there
> > > > another place that validates the firmware signatures.  I'm not seeing
> > > > which patch requires firmware to be signed?
> > > 
> > > Luis has a set of patches for this.  However, I'm not sure if that's going
> > > anywhere at the moment.  Possibly I should remove this from the manpage for
> > > the moment.
> 
> Remove it for now. The state of of affairs for firmware signing is complex given
> that we first wanted to address how to properly grow the API without making
> the API worse. This in and of itself was an effort, and that effort also
> evaluated two different development paradigms:
> 
> 	o functional API
> 	o data driven API
> 
> I only recently was convinced that functional API should be used, even for
> commonly used exported symbols,

Are you?
I haven't answered Linus' question, but my concern about functional APIs,
as far as firmware signing goes, is that we have no way to _enforce_
firmware signing to existing (i.e. verification-unaware) drivers if we need
an explicit call of a function, say, verify_firmware().

> and as such I've been going back and slowly
> grooming the firmware API with small atomic changes to first clean up the
> complex flag mess we have.
> 
> Since I'm busy with that Takahiro AKASHI has taken up firmware singing effort
> but this will depend on the above small cleanup to be done first. I was busy
> with addressing existing bugs on the firmware API for a while, then company
> travel / conferences so was not able to address this, but I'm back now and
> I believe I should be able to tackle the cleanup now.

Good to hear.

> Only after this is merged can we expect a final respin of the firmware signing
> effort.
> 
> > Or reflect that IMA-appraisal, if enabled, will enforce firmware being
> > validly signed.
> 
> But FWICT lockdown is a built-in kernel thingy, unless lockdown implies IMA
> it would not be the place to refer to it.

I think that the situation is the same as in module signing.

-Takahiro AKASHI

> It seems the documentation was proposed to help users if an error was caught.
> That error should cover only what is being addressed in code on the kernel.
> 
>   Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Wed, Nov 08, 2017 at 03:15:54PM +0900, AKASHI, Takahiro wrote:
> Luis,
> 
> Thank you for this heads-up.
> 
> On Wed, Nov 08, 2017 at 12:07:00AM +0100, Luis R. Rodriguez wrote:
> > On Thu, Nov 02, 2017 at 06:10:41PM -0400, Mimi Zohar wrote:
> > > On Thu, 2017-11-02 at 22:04 +0000, David Howells wrote:
> > > > Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> > > > 
> > > > > > Only validly signed device firmware may be loaded.
> > > > > 
> > > > > fw_get_filesystem_firmware() calls kernel_read_file_from_path() to
> > > > > read the firmware, which calls into the security hooks. Is there
> > > > > another place that validates the firmware signatures.  I'm not seeing
> > > > > which patch requires firmware to be signed?
> > > > 
> > > > Luis has a set of patches for this.  However, I'm not sure if that's going
> > > > anywhere at the moment.  Possibly I should remove this from the manpage for
> > > > the moment.
> > 
> > Remove it for now. The state of of affairs for firmware signing is complex given
> > that we first wanted to address how to properly grow the API without making
> > the API worse. This in and of itself was an effort, and that effort also
> > evaluated two different development paradigms:
> > 
> > 	o functional API
> > 	o data driven API
> > 
> > I only recently was convinced that functional API should be used, even for
> > commonly used exported symbols,
> 
> Are you?

Yes, this stemmed from the fact that even system calls can be abused through
data driven APIs, and that long term a functional API at least can make
evolutions much easier to review and bisect.

That said this is all based on *empirical hearsay*, and no formal observations.
But the ease to more easily bisect long term is enough for me to consider this 
for the firmware API given subtle regressions have been a long standing pain
on the firmware API and I would not want to make the process of bisecting
any harder.

If anyone *does* have actual efforts which compares and contrasts both, I'd
love to get them, to further back my current position, but as-is I'm already
sold on functional API driven interface.

> I haven't answered Linus' question, but my concern about functional APIs,
> as far as firmware signing goes, is that we have no way to _enforce_
> firmware signing to existing (i.e. verification-unaware) drivers if we need
> an explicit call of a function, say, verify_firmware().

Your concern seems to be that a functional driven API for firmware signing would
implicate having to support verify_firmware() for drivers which *cannot* get
signed firmware verified, is that correct?

IMHO that should just fail then, ie, a "locked down" kernel should not want to
*pass* a firmware signature if such thing could not be done.

Its no different than trying to verify a signed module on a "locked down" for
which it has no signature.

But perhaps I'm not understanding the issue well, let me know.

> > and as such I've been going back and slowly
> > grooming the firmware API with small atomic changes to first clean up the
> > complex flag mess we have.
> > 
> > Since I'm busy with that Takahiro AKASHI has taken up firmware singing effort
> > but this will depend on the above small cleanup to be done first. I was busy
> > with addressing existing bugs on the firmware API for a while, then company
> > travel / conferences so was not able to address this, but I'm back now and
> > I believe I should be able to tackle the cleanup now.
> 
> Good to hear.
> 
> > Only after this is merged can we expect a final respin of the firmware signing
> > effort.
> > 
> > > Or reflect that IMA-appraisal, if enabled, will enforce firmware being
> > > validly signed.
> > 
> > But FWICT lockdown is a built-in kernel thingy, unless lockdown implies IMA
> > it would not be the place to refer to it.
> 
> I think that the situation is the same as in module signing.

But by definition a "locked down" kernel will enforce module signing, and it
would seem logical that if modules signing is enforced, eventually a statement
about firmware signing can be added, as it relates to a kernel enforcement
effort, not a kernel-userpace framework such as IMA.

  Luis

> -Takahiro AKASHI
> 
> > It seems the documentation was proposed to help users if an error was caught.
> > That error should cover only what is being addressed in code on the kernel.
> > 
> >   Luis
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[AKASHI, Takahiro]

On Wed, Nov 08, 2017 at 08:46:26PM +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 08, 2017 at 03:15:54PM +0900, AKASHI, Takahiro wrote:
> > Luis,
> > 
> > Thank you for this heads-up.
> > 
> > On Wed, Nov 08, 2017 at 12:07:00AM +0100, Luis R. Rodriguez wrote:
> > > On Thu, Nov 02, 2017 at 06:10:41PM -0400, Mimi Zohar wrote:
> > > > On Thu, 2017-11-02 at 22:04 +0000, David Howells wrote:
> > > > > Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> > > > > 
> > > > > > > Only validly signed device firmware may be loaded.
> > > > > > 
> > > > > > fw_get_filesystem_firmware() calls kernel_read_file_from_path() to
> > > > > > read the firmware, which calls into the security hooks. Is there
> > > > > > another place that validates the firmware signatures.  I'm not seeing
> > > > > > which patch requires firmware to be signed?
> > > > > 
> > > > > Luis has a set of patches for this.  However, I'm not sure if that's going
> > > > > anywhere at the moment.  Possibly I should remove this from the manpage for
> > > > > the moment.
> > > 
> > > Remove it for now. The state of of affairs for firmware signing is complex given
> > > that we first wanted to address how to properly grow the API without making
> > > the API worse. This in and of itself was an effort, and that effort also
> > > evaluated two different development paradigms:
> > > 
> > > 	o functional API
> > > 	o data driven API
> > > 
> > > I only recently was convinced that functional API should be used, even for
> > > commonly used exported symbols,
> > 
> > Are you?
> 
> Yes, this stemmed from the fact that even system calls can be abused through
> data driven APIs, and that long term a functional API at least can make
> evolutions much easier to review and bisect.
> 
> That said this is all based on *empirical hearsay*, and no formal observations.
> But the ease to more easily bisect long term is enough for me to consider this 
> for the firmware API given subtle regressions have been a long standing pain
> on the firmware API and I would not want to make the process of bisecting
> any harder.
> 
> If anyone *does* have actual efforts which compares and contrasts both, I'd
> love to get them, to further back my current position, but as-is I'm already
> sold on functional API driven interface.
> 
> > I haven't answered Linus' question, but my concern about functional APIs,
> > as far as firmware signing goes, is that we have no way to _enforce_
> > firmware signing to existing (i.e. verification-unaware) drivers if we need
> > an explicit call of a function, say, verify_firmware().
> 
> Your concern seems to be that a functional driven API for firmware signing would
> implicate having to support verify_firmware() for drivers which *cannot* get
> signed firmware verified, is that correct?
> 
> IMHO that should just fail then, ie, a "locked down" kernel should not want to
> *pass* a firmware signature if such thing could not be done.
> 
> Its no different than trying to verify a signed module on a "locked down" for
> which it has no signature.
> 
> But perhaps I'm not understanding the issue well, let me know.

My point is quite simple:
my_deviceA_init() {
        err = request_firmware(&fw, "deviceA"); <--- (a)
        if (err)
                goto err_request;

        err = verify_firmware(fw);  <--- (b)
        if (err)
                goto err_verify;

        load_fw_to_deviceA(fw);     <--- (c)
        ...
}

As legacy device drivers does not have (b), there is no chance to
prevent loading a firmware at (c) for locked-down kernel.

If you allow me to bring in yet another function, say
request_firmware_signable(), which should be used in place of (a)
for all verification-aware drivers, that would be fine.
In this case, all the invocation of request_firmware() in legacy code
could be forced to fail in locked-down kernel.

But I think that "signable" should be allowed to be combined with other
features of request_firmware variants like _(no)wait or _direct.

-Takahiro AKASHI

> 
> > > and as such I've been going back and slowly
> > > grooming the firmware API with small atomic changes to first clean up the
> > > complex flag mess we have.
> > > 
> > > Since I'm busy with that Takahiro AKASHI has taken up firmware singing effort
> > > but this will depend on the above small cleanup to be done first. I was busy
> > > with addressing existing bugs on the firmware API for a while, then company
> > > travel / conferences so was not able to address this, but I'm back now and
> > > I believe I should be able to tackle the cleanup now.
> > 
> > Good to hear.
> > 
> > > Only after this is merged can we expect a final respin of the firmware signing
> > > effort.
> > > 
> > > > Or reflect that IMA-appraisal, if enabled, will enforce firmware being
> > > > validly signed.
> > > 
> > > But FWICT lockdown is a built-in kernel thingy, unless lockdown implies IMA
> > > it would not be the place to refer to it.
> > 
> > I think that the situation is the same as in module signing.
> 
> But by definition a "locked down" kernel will enforce module signing, and it
> would seem logical that if modules signing is enforced, eventually a statement
> about firmware signing can be added, as it relates to a kernel enforcement
> effort, not a kernel-userpace framework such as IMA.
> 
>   Luis
> 
> > -Takahiro AKASHI
> > 
> > > It seems the documentation was proposed to help users if an error was caught.
> > > That error should cover only what is being addressed in code on the kernel.
> > > 
> > >   Luis
> > 
> 
> -- 
> Luis Rodriguez, SUSE LINUX GmbH
> Maxfeldstrasse 5; D-90409 Nuernberg

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

> > IMHO that should just fail then, ie, a "locked down" kernel should not want to
> > *pass* a firmware signature if such thing could not be done.
> > 
> > Its no different than trying to verify a signed module on a "locked down" for
> > which it has no signature.
> > 
> > But perhaps I'm not understanding the issue well, let me know.
> 
> My point is quite simple:
> my_deviceA_init() {
>         err = request_firmware(&fw, "deviceA"); <--- (a)
>         if (err)
>                 goto err_request;
> 
>         err = verify_firmware(fw);  <--- (b)
>         if (err)
>                 goto err_verify;
> 
>         load_fw_to_deviceA(fw);     <--- (c)
>         ...
> }
> 
> As legacy device drivers does not have (b), there is no chance to
> prevent loading a firmware at (c) for locked-down kernel.
> 
> If you allow me to bring in yet another function, say
> request_firmware_signable(), which should be used in place of (a)
> for all verification-aware drivers, that would be fine.

I really don't understand why you need a new function.  The
request_firmware() eventually calls kernel_read_file_from_path(),
which already calls the pre and post LSM hooks.

IMA-appraisal is already on these hooks verifying the requested
firmware's signature.  For systems with "lockdown" enabled, but
without IMA-appraisal enabled, define a small, builtin LSM that sits
on these LSM hooks and denies the unsigned firmware requests.

Mimi

> In this case, all the invocation of request_firmware() in legacy code
> could be forced to fail in locked-down kernel.
> 
> But I think that "signable" should be allowed to be combined with other
> features of request_firmware variants like _(no)wait or _direct.
> 
> -Takahiro AKASHI

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[AKASHI, Takahiro]

Mimi,

On Wed, Nov 08, 2017 at 09:17:37PM -0500, Mimi Zohar wrote:
> > > IMHO that should just fail then, ie, a "locked down" kernel should not want to
> > > *pass* a firmware signature if such thing could not be done.
> > > 
> > > Its no different than trying to verify a signed module on a "locked down" for
> > > which it has no signature.
> > > 
> > > But perhaps I'm not understanding the issue well, let me know.
> > 
> > My point is quite simple:
> > my_deviceA_init() {
> >         err = request_firmware(&fw, "deviceA"); <--- (a)
> >         if (err)
> >                 goto err_request;
> > 
> >         err = verify_firmware(fw);  <--- (b)
> >         if (err)
> >                 goto err_verify;
> > 
> >         load_fw_to_deviceA(fw);     <--- (c)
> >         ...
> > }
> > 
> > As legacy device drivers does not have (b), there is no chance to
> > prevent loading a firmware at (c) for locked-down kernel.
> > 
> > If you allow me to bring in yet another function, say
> > request_firmware_signable(), which should be used in place of (a)
> > for all verification-aware drivers, that would be fine.
> 
> I really don't understand why you need a new function.  The
> request_firmware() eventually calls kernel_read_file_from_path(),
> which already calls the pre and post LSM hooks.

My assumption here is:
1) there are some less important and so security-insensitive firmwares,
   by which I mean that such firmwares won't be expected to be signed in
   terms of vulnerability or integrity.
   (I can't give you examples though.)
2) firmware's signature will be presented separately from the firmware
   blob itself. Say, "firmware.bin.p7s" for "firmware.bin"

I don't think that the current security_kernel(_post)_read_file() scheme
fit with this assumption very well.

Thanks,
-Takahiro AKASHI


> IMA-appraisal is already on these hooks verifying the requested
> firmware's signature.  For systems with "lockdown" enabled, but
> without IMA-appraisal enabled, define a small, builtin LSM that sits
> on these LSM hooks and denies the unsigned firmware requests.
> 
> Mimi
> 
> > In this case, all the invocation of request_firmware() in legacy code
> > could be forced to fail in locked-down kernel.
> > 
> > But I think that "signable" should be allowed to be combined with other
> > features of request_firmware variants like _(no)wait or _direct.
> > 
> > -Takahiro AKASHI
> 

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Thu, 2017-11-09 at 13:46 +0900, AKASHI, Takahiro wrote:
> Mimi,
> 
> On Wed, Nov 08, 2017 at 09:17:37PM -0500, Mimi Zohar wrote:
> > > > IMHO that should just fail then, ie, a "locked down" kernel should not want to
> > > > *pass* a firmware signature if such thing could not be done.
> > > > 
> > > > Its no different than trying to verify a signed module on a "locked down" for
> > > > which it has no signature.
> > > > 
> > > > But perhaps I'm not understanding the issue well, let me know.
> > > 
> > > My point is quite simple:
> > > my_deviceA_init() {
> > >         err = request_firmware(&fw, "deviceA"); <--- (a)
> > >         if (err)
> > >                 goto err_request;
> > > 
> > >         err = verify_firmware(fw);  <--- (b)
> > >         if (err)
> > >                 goto err_verify;
> > > 
> > >         load_fw_to_deviceA(fw);     <--- (c)
> > >         ...
> > > }
> > > 
> > > As legacy device drivers does not have (b), there is no chance to
> > > prevent loading a firmware at (c) for locked-down kernel.
> > > 
> > > If you allow me to bring in yet another function, say
> > > request_firmware_signable(), which should be used in place of (a)
> > > for all verification-aware drivers, that would be fine.
> > 
> > I really don't understand why you need a new function.  The
> > request_firmware() eventually calls kernel_read_file_from_path(),
> > which already calls the pre and post LSM hooks.
> 
> My assumption here is:
> 1) there are some less important and so security-insensitive firmwares,
>    by which I mean that such firmwares won't be expected to be signed in
>    terms of vulnerability or integrity.
>    (I can't give you examples though.)

Differentiating between firmware can be achieved based on LSM labels,
or other file metadata.  Whether this is acceptable for "lockdown"
mode is a separate issue. 

> 2) firmware's signature will be presented separately from the firmware
>    blob itself. Say, "firmware.bin.p7s" for "firmware.bin"

Currently there are appended signatures and signatures stored as
xattrs. (Thiago Bauermann posted patches adding appending signature
support within IMA.)  Adding support for detached signatures could be
added as well, but it should be defined within the existing integrity
framework.

Mimi

> I don't think that the current security_kernel(_post)_read_file() scheme
> fit with this assumption very well.
> 
> Thanks,
> -Takahiro AKASHI
> 
> 
> > IMA-appraisal is already on these hooks verifying the requested
> > firmware's signature.  For systems with "lockdown" enabled, but
> > without IMA-appraisal enabled, define a small, builtin LSM that sits
> > on these LSM hooks and denies the unsigned firmware requests.
> > 
> > Mimi
> > 
> > > In this case, all the invocation of request_firmware() in legacy code
> > > could be forced to fail in locked-down kernel.
> > > 
> > > But I think that "signable" should be allowed to be combined with other
> > > features of request_firmware variants like _(no)wait or _direct.
> > > 
> > > -Takahiro AKASHI
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Alan Cox]

> My assumption here is:
> 1) there are some less important and so security-insensitive firmwares,
>    by which I mean that such firmwares won't be expected to be signed in
>    terms of vulnerability or integrity.
>    (I can't give you examples though.)
> 2) firmware's signature will be presented separately from the firmware
>    blob itself. Say, "firmware.bin.p7s" for "firmware.bin"

For x86 at least any firmware on any system modern enough to support
'secure' boot should already be signed. The only major exception is
likely to be for things like random USB widgets.

Even things like input controller firmware loaded over i2c or spi
is usually signed because you could do fun things with input faking
otherwise.

The other usual exception is FPGAs, but since the point of an FPGA is
usually the fact it *can* be reprogrammed it's not clear that signing
FPGA firmware makes sense unless it is designed to be fixed function.

You can't subvert the bus protocols on the x86 FPGA I am aware of as
those bits are signed (or hard IP), but without IOMMU I am not sure FPGA
and 'secure' boot is completely compatible.

Alan

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Sat, 2017-11-11 at 02:32 +0000, Alan Cox wrote:
> > My assumption here is:
> > 1) there are some less important and so security-insensitive firmwares,
> >    by which I mean that such firmwares won't be expected to be signed in
> >    terms of vulnerability or integrity.
> >    (I can't give you examples though.)
> > 2) firmware's signature will be presented separately from the firmware
> >    blob itself. Say, "firmware.bin.p7s" for "firmware.bin"
> 
> For x86 at least any firmware on any system modern enough to support
> 'secure' boot should already be signed. The only major exception is
> likely to be for things like random USB widgets.

Does this mean that the firmware signature is contained within the
firmware?  On Linux, is anything verifying this signature?  Or is this
the hw verifying the firmware's signature?

> Even things like input controller firmware loaded over i2c or spi
> is usually signed because you could do fun things with input faking
> otherwise.
> 
> The other usual exception is FPGAs, but since the point of an FPGA is
> usually the fact it *can* be reprogrammed it's not clear that signing
> FPGA firmware makes sense unless it is designed to be fixed function.
> 
> You can't subvert the bus protocols on the x86 FPGA I am aware of as
> those bits are signed (or hard IP), but without IOMMU I am not sure FPGA
> and 'secure' boot is completely compatible.

Ok, but why differentiate between "fixed" and research/development
FPGA functions?  In both cases, the FPGA firmware would need to be
signed locally.  Wouldn't this be similar to kernel developers
building their own kernels and signing the kernel modules?

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Sat, Nov 11, 2017 at 02:32:40AM +0000, Alan Cox wrote:
> > My assumption here is:
> > 1) there are some less important and so security-insensitive firmwares,
> >    by which I mean that such firmwares won't be expected to be signed in
> >    terms of vulnerability or integrity.
> >    (I can't give you examples though.)
> > 2) firmware's signature will be presented separately from the firmware
> >    blob itself. Say, "firmware.bin.p7s" for "firmware.bin"
> 
> For x86 at least any firmware on any system modern enough to support
> 'secure' boot should already be signed. The only major exception is
> likely to be for things like random USB widgets.

Alan, the firmware being considered here is /lib/firmware firmware, which
is not signed today. Its unclear to me how you mean that /lib/firmware files
are already signed or verified today.

> The other usual exception is FPGAs, but since the point of an FPGA is
> usually the fact it *can* be reprogrammed it's not clear that signing
> FPGA firmware makes sense unless it is designed to be fixed function.

Agreed that we may end up with exemptions where we purposely cannot
get signed firmware for, or where for certain drivers this makes no
sense.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Alan Cox]

On Mon, 13 Nov 2017 18:42:50 +0100
"Luis R. Rodriguez" <mcgrof@kernel.org> wrote:

> On Sat, Nov 11, 2017 at 02:32:40AM +0000, Alan Cox wrote:
> > > My assumption here is:
> > > 1) there are some less important and so security-insensitive firmwares,
> > >    by which I mean that such firmwares won't be expected to be signed in
> > >    terms of vulnerability or integrity.
> > >    (I can't give you examples though.)
> > > 2) firmware's signature will be presented separately from the firmware
> > >    blob itself. Say, "firmware.bin.p7s" for "firmware.bin"  
> > 
> > For x86 at least any firmware on any system modern enough to support
> > 'secure' boot should already be signed. The only major exception is
> > likely to be for things like random USB widgets.  
> 
> Alan, the firmware being considered here is /lib/firmware firmware, which
> is not signed today. Its unclear to me how you mean that /lib/firmware files
> are already signed or verified today

By the hardware they get loaded onto. Pretty much all of the hardware on
PC class systems at least is signed and checked by the platform or the
device itself. For a 'secure' boot era PC that's pretty much everything
except USB toys and FPGA.

So you don't actually need to sign a lot of PC class firmware because
it's already signed.

Alan

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Mon, Nov 13, 2017 at 09:08:48PM +0000, Alan Cox wrote:
> On Mon, 13 Nov 2017 18:42:50 +0100
> "Luis R. Rodriguez" <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> > On Sat, Nov 11, 2017 at 02:32:40AM +0000, Alan Cox wrote:
> > > > My assumption here is:
> > > > 1) there are some less important and so security-insensitive firmwares,
> > > >    by which I mean that such firmwares won't be expected to be signed in
> > > >    terms of vulnerability or integrity.
> > > >    (I can't give you examples though.)
> > > > 2) firmware's signature will be presented separately from the firmware
> > > >    blob itself. Say, "firmware.bin.p7s" for "firmware.bin"  
> > > 
> > > For x86 at least any firmware on any system modern enough to support
> > > 'secure' boot should already be signed. The only major exception is
> > > likely to be for things like random USB widgets.  
> > 
> > Alan, the firmware being considered here is /lib/firmware firmware, which
> > is not signed today. Its unclear to me how you mean that /lib/firmware files
> > are already signed or verified today
> 
> By the hardware they get loaded onto. Pretty much all of the hardware on
> PC class systems at least is signed and checked by the platform or the
> device itself. For a 'secure' boot era PC that's pretty much everything
> except USB toys and FPGA.
> 
> So you don't actually need to sign a lot of PC class firmware because
> it's already signed.

It would seem that this logic can trump the need for /lib/firmware signing. And
for users where this is *not done* they can open code it, just as is being done
for cfg80211 to help a new kernel config replace CRDA. Additionally for non-UEFI
platforms which *also* need firmware signing, one viable option is to rely
on IMA.

If we have enough open-coded uses similar to cfg80211 we can revisit this
question  later.

I am curious though, is the above notion of having hardware require signed
firmware an implication brought down by UEFI? If so do you have any pointers
to where this is stipulated? Or is it just a best practice we assume some
manufacturers are implementing?

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Alan Cox]

> I am curious though, is the above notion of having hardware require signed
> firmware an implication brought down by UEFI? If so do you have any pointers
> to where this is stipulated? Or is it just a best practice we assume some
> manufacturers are implementing?

It's a mix of best practice and meeting the so called 'secure boot'
requirements. In the non Linux space exactly the same problems exist in
terms of trusting devices and firmware, building a root of trust and even
more so when producing 'hardened' platforms.

Some stuff isn't - USB devices for example don't get to pee on random
memory so often isn't signed.

Alan

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[David Howells]

Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:

> So you don't actually need to sign a lot of PC class firmware because
> it's already signed.

Whilst that may be true, we either have to check signatures on every bit of
firmware that the appropriate driver doesn't say is meant to be signed or not
bother.

David

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Mon, Nov 13, 2017 at 1:44 PM, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Whilst that may be true, we either have to check signatures on every bit of
> firmware that the appropriate driver doesn't say is meant to be signed or not
> bother.

I vote for "not bother".

Seriously, if you have firmware in /lib/firmware, and you don't trust
it, what the hell are you doing?

Oh, it's one of those "let's protect people from themselves, so that
they can't possibly break Disney^W^W be terrorists - but but the
children" things again, isn't it?

Watch me care.

                  Linus

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Alan Cox]

On Mon, 13 Nov 2017 14:09:10 -0800
Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:

> On Mon, Nov 13, 2017 at 1:44 PM, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > Whilst that may be true, we either have to check signatures on every bit of
> > firmware that the appropriate driver doesn't say is meant to be signed or not
> > bother.  
> 
> I vote for "not bother".
> 
> Seriously, if you have firmware in /lib/firmware, and you don't trust
> it, what the hell are you doing?

Booting a computer 8) - does anyone trust firmware ???


That aside if you can replace the firmware with your own, and your own
firmware compromises the system then you have a privilege escalation from
CAP_SYS_DAC to CAP_SYS_RAWIO just as you do with unsigned modules

(you can't load modules without lots of rights but who cares because you
can simply use cp to put them there with lesser rights and let the kernel
do it).


Alan

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Mon, 2017-11-13 at 14:09 -0800, Linus Torvalds wrote:
> On Mon, Nov 13, 2017 at 1:44 PM, David Howells <dhowells@redhat.com> wrote:
> >
> > Whilst that may be true, we either have to check signatures on every bit of
> > firmware that the appropriate driver doesn't say is meant to be signed or not
> > bother.
> 
> I vote for "not bother".
> 
> Seriously, if you have firmware in /lib/firmware, and you don't trust
> it, what the hell are you doing?

I might "trust" the files in /lib/firmware, but I also want to make
sure that they haven't changed.  File signatures provide file
provenance and integrity guarantees.

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Greg Kroah-Hartman]

On Tue, Nov 14, 2017 at 07:21:38AM -0500, Mimi Zohar wrote:
> On Mon, 2017-11-13 at 14:09 -0800, Linus Torvalds wrote:
> > On Mon, Nov 13, 2017 at 1:44 PM, David Howells <dhowells@redhat.com> wrote:
> > >
> > > Whilst that may be true, we either have to check signatures on every bit of
> > > firmware that the appropriate driver doesn't say is meant to be signed or not
> > > bother.
> > 
> > I vote for "not bother".
> > 
> > Seriously, if you have firmware in /lib/firmware, and you don't trust
> > it, what the hell are you doing?
> 
> I might "trust" the files in /lib/firmware, but I also want to make
> sure that they haven't changed.  File signatures provide file
> provenance and integrity guarantees.

Then "verify" them with signatures that you generate yourself.  Like
dm-verify does for the partition that you put the firmware on.

thanks,

greg k-h

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Tue, 2017-11-14 at 13:38 +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 14, 2017 at 07:21:38AM -0500, Mimi Zohar wrote:
> > On Mon, 2017-11-13 at 14:09 -0800, Linus Torvalds wrote:
> > > On Mon, Nov 13, 2017 at 1:44 PM, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > >
> > > > Whilst that may be true, we either have to check signatures on every bit of
> > > > firmware that the appropriate driver doesn't say is meant to be signed or not
> > > > bother.
> > > 
> > > I vote for "not bother".
> > > 
> > > Seriously, if you have firmware in /lib/firmware, and you don't trust
> > > it, what the hell are you doing?
> > 
> > I might "trust" the files in /lib/firmware, but I also want to make
> > sure that they haven't changed.  File signatures provide file
> > provenance and integrity guarantees.
> 
> Then "verify" them with signatures that you generate yourself.  Like
> dm-verify does for the partition that you put the firmware on.

The discussion, here, is in the context of the "lockdown" patch set,
without IMA-appraisal configured.  Kernel modules and the kexec kernel
image require file signatures in lockdown mode.  An equivalent method
of requiring file signatures for firmware (without IMA-appraisal) does
not exist.

I posted the patch [RFC PATCH v2] "fw_lockdown: new micro LSM module
to prevent loading unsigned firmware".  The patch and discussion can
be found here - (https://lkml.org/lkml/2017/11/13/217).

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Tue, Nov 14, 2017 at 4:21 AM, Mimi Zohar <zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> On Mon, 2017-11-13 at 14:09 -0800, Linus Torvalds wrote:
>>
>> Seriously, if you have firmware in /lib/firmware, and you don't trust
>> it, what the hell are you doing?
>
> I might "trust" the files in /lib/firmware, but I also want to make
> sure that they haven't changed.  File signatures provide file
> provenance and integrity guarantees.

Sure. But that has absolutely nothing to do with "firmware".

It is equally true of /usr/bin/* and pretty much everything in the system.

It's this insane "firmware is special" that I disagree with. It's not
special at all.

                Linus

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Matthew Garrett]

On Tue, Nov 14, 2017 at 9:34 AM, Linus Torvalds
<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> It's this insane "firmware is special" that I disagree with. It's not
> special at all.

Our ability to determine that userland hasn't been tampered with
depends on the kernel being trustworthy. If userland can upload
arbitrary firmware to DMA-capable devices then we can no longer trust
the kernel. So yes, firmware is special.

Here's an example: we have a signed initramfs that's loaded by a
signed bootloader. That initramfs sets up a trustworthy audit chain
and loads an LSM policy that prevents the rest of userland from
interfering with it. From that point on, we don't care about the rest
of userland being signed - we know it can't interfere with us, but we
can reliably inspect what it's doing. Even an offline attack can't do
any damage, since the audit code is still signed. However, the
LSM-imposed boundary depends on the kernel being trustworthy. If an
attacker can replace the firmware that's uploaded to a device that can
do arbitrary DMA then they can tamper with the supposedly trustworthy
audit code and provide false information. Being able to tamper with
the contents of /usr/bin/* doesn't give them that.

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Tue, Nov 14, 2017 at 11:58 AM, Matthew Garrett <mjg59-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> Our ability to determine that userland hasn't been tampered with
> depends on the kernel being trustworthy. If userland can upload
> arbitrary firmware to DMA-capable devices then we can no longer trust
> the kernel. So yes, firmware is special.

You're ignoring the whole "firmware is already signed by the hardware
manufacturer and we don't even have access to it" part.

You're also ignoring the fact that we can't trust firmware _anyway_,
as Alan pointed out.

Seriously. Some of the worst security issues have been with exactly
the fact that we can't trust the hardware to begin with, where
firmware/hardware combinations are not trusted to begin with.

This is all theoretical security masturbation. The _real_ attacks have
been elsewhere.

               Linus

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Tue, Nov 14, 2017 at 12:18:54PM -0800, Linus Torvalds wrote:
> This is all theoretical security masturbation. The _real_ attacks have
> been elsewhere.

In my research on this front I'll have to agree with this, in terms of
justification and there are only *two* arguments which I've so far have found
to justify firmware signing:

a) If you want signed modules, you therefore should want signed firmware.
   This however seems to be solved by using trusted boot thing, given it
   seems trusted boot requires having firmware be signed as well. (Docs
   would be useful to get about where in the specs this is mandated,
   anyone?). Are there platforms that don't have trusted boot or for which
   they don't enforce hardware checking for signed firmware for which
   we still want to support firmware signing for? Are there platforms
   that require and use module signing but don't and won't have a trusted
   boot of some sort? Do we care?

b) Other kernel components have implemented some sort of "signing" a file
   prior to sending it to the kernel. One is wifi for the regulatory.bin,
   through CRDA, and this helped us persuade certain folks to support wifi
   with open drivers. We can easily replace CRDA with kernel fetch for a
   signed file given most of the APIs for this is available.

Originally effort a) was done for b), as it was believed we needed a).
If we are saying a) is pointless, what about b)? A simple new
kernel_read_file_from_path_pkcs7_signed() or some such would suffice.

Johannes made cfg80211 recently just use request_firmware() now via commit on
linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
he got tired of waiting firmware signing, but note he implemented a signature
checking on its own so he open codes verify_pkcs7_signature() after the
request_firmware() call. If we are happy to live with this, then so be it.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Matthew Garrett]

On Tue, Nov 14, 2017 at 3:50 PM, Luis R. Rodriguez <mcgrof-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Nov 14, 2017 at 12:18:54PM -0800, Linus Torvalds wrote:
>> This is all theoretical security masturbation. The _real_ attacks have
>> been elsewhere.
>
> In my research on this front I'll have to agree with this, in terms of
> justification and there are only *two* arguments which I've so far have found
> to justify firmware signing:
>
> a) If you want signed modules, you therefore should want signed firmware.
>    This however seems to be solved by using trusted boot thing, given it
>    seems trusted boot requires having firmware be signed as well. (Docs
>    would be useful to get about where in the specs this is mandated,
>    anyone?). Are there platforms that don't have trusted boot or for which
>    they don't enforce hardware checking for signed firmware for which
>    we still want to support firmware signing for? Are there platforms
>    that require and use module signing but don't and won't have a trusted
>    boot of some sort? Do we care?

TPM-backed Trusted Boot means you don't /need/ to sign anything, since
the measurements of what you loaded will end up in the TPM. But
signatures make it a lot easier, since you can just assert that only
signed material will be loaded and so you only need to measure the
kernel and the trusted keys.

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[James Bottomley]

On Tue, 2017-11-14 at 15:55 -0500, Matthew Garrett wrote:
> On Tue, Nov 14, 2017 at 3:50 PM, Luis R. Rodriguez <mcgrof@kernel.org
> > wrote:
> > 
> > On Tue, Nov 14, 2017 at 12:18:54PM -0800, Linus Torvalds wrote:
> > > 
> > > This is all theoretical security masturbation. The _real_ attacks
> > > have been elsewhere.
> > 
> > In my research on this front I'll have to agree with this, in terms
> > of justification and there are only *two* arguments which I've so 
> > far have found to justify firmware signing:
> > 
> > a) If you want signed modules, you therefore should want signed
> > firmware.
> >    This however seems to be solved by using trusted boot thing,
> > given it
> >    seems trusted boot requires having firmware be signed as well.
> > (Docs
> >    would be useful to get about where in the specs this is
> > mandated,
> >    anyone?). Are there platforms that don't have trusted boot or
> > for which
> >    they don't enforce hardware checking for signed firmware for
> > which
> >    we still want to support firmware signing for? Are there
> > platforms
> >    that require and use module signing but don't and won't have a
> > trusted
> >    boot of some sort? Do we care?
> 
> TPM-backed Trusted Boot means you don't /need/ to sign anything,
> since the measurements of what you loaded will end up in the TPM. But
> signatures make it a lot easier, since you can just assert that only
> signed material will be loaded and so you only need to measure the
> kernel and the trusted keys.

Actually, I'd disagree with that quite a lot: measured boot only works
if you're attesting to something outside of your system that has the
capability for doing something about a wrong measurement.  Absent that,
measured boot has no safety whatsoever.  Secure boot, on the other
hand, can enforce not booting with elements that fail the signature
check.

The question, really, in any system, is how you want to prove security.
 In a standalone server system, measured boot is pretty useless because
you don't have an external entity to attest to, so signatures and
secure boot are really the bulwark against breaches.  In a properly
attested server cluster whose attestation controller has the ability to
reboot you, perhaps signatures and secure boot don't add that much more
value.

James

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Matthew Garrett]

On Tue, Nov 14, 2017 at 2:14 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Tue, 2017-11-14 at 15:55 -0500, Matthew Garrett wrote:
>> TPM-backed Trusted Boot means you don't /need/ to sign anything,
>> since the measurements of what you loaded will end up in the TPM. But
>> signatures make it a lot easier, since you can just assert that only
>> signed material will be loaded and so you only need to measure the
>> kernel and the trusted keys.
>
> Actually, I'd disagree with that quite a lot: measured boot only works
> if you're attesting to something outside of your system that has the
> capability for doing something about a wrong measurement.  Absent that,
> measured boot has no safety whatsoever.  Secure boot, on the other
> hand, can enforce not booting with elements that fail the signature
> check.

Measured boot has a great deal of value in the sealing of private
material, even in the absence of attestation. The way Microsoft make
use of PCR7 is a good example of how signatures make this easier -
achieving the same goal with a full measurement of the boot chain
instead of relying on signature validation results in significantly
more fragility.

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[James Bottomley]

On Tue, 2017-11-14 at 14:17 -0800, Matthew Garrett wrote:
> On Tue, Nov 14, 2017 at 2:14 PM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > 
> > On Tue, 2017-11-14 at 15:55 -0500, Matthew Garrett wrote:
> > > 
> > > TPM-backed Trusted Boot means you don't /need/ to sign anything,
> > > since the measurements of what you loaded will end up in the TPM.
> > > But signatures make it a lot easier, since you can just assert
> > > that only signed material will be loaded and so you only need to
> > > measure the kernel and the trusted keys.
> > 
> > Actually, I'd disagree with that quite a lot: measured boot only
> > works if you're attesting to something outside of your system that
> > has the capability for doing something about a wrong
> > measurement.  Absent that, measured boot has no safety
> > whatsoever.  Secure boot, on the other hand, can enforce not
> > booting with elements that fail the signature check.
> 
> Measured boot has a great deal of value in the sealing of private
> material, even in the absence of attestation. The way Microsoft make
> use of PCR7 is a good example of how signatures make this easier -
> achieving the same goal with a full measurement of the boot chain
> instead of relying on signature validation results in significantly
> more fragility.

OK, so I agree that if you have sealed something required for boot (and
have the capability for resealing it on OS upgrade) you can use
measurements locally.  However, I don't believe we have any systems
today in Linux which can do this (we have theoretical ideas about how
we might do it with LUKS root keys and one day we might actually have
the infrastructure to make it viable for a standard laptop).

Absent that, secure boot provides a reasonable measure of security
which works with today's infrastructure.

Note: this doesn't mean I necessarily want signatures everywhere (like
firmware).  We can sign elements in blobs that provide the effective
security without needing more granular signatures.

James

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Matthew Garrett]

On Tue, Nov 14, 2017 at 2:31 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Tue, 2017-11-14 at 14:17 -0800, Matthew Garrett wrote:
>> Measured boot has a great deal of value in the sealing of private
>> material, even in the absence of attestation. The way Microsoft make
>> use of PCR7 is a good example of how signatures make this easier -
>> achieving the same goal with a full measurement of the boot chain
>> instead of relying on signature validation results in significantly
>> more fragility.
>
> OK, so I agree that if you have sealed something required for boot (and
> have the capability for resealing it on OS upgrade) you can use
> measurements locally.  However, I don't believe we have any systems
> today in Linux which can do this (we have theoretical ideas about how
> we might do it with LUKS root keys and one day we might actually have
> the infrastructure to make it viable for a standard laptop).

It's used for TPMTOTP, for instance.

> Absent that, secure boot provides a reasonable measure of security
> which works with today's infrastructure.
>
> Note: this doesn't mean I necessarily want signatures everywhere (like
> firmware).  We can sign elements in blobs that provide the effective
> security without needing more granular signatures.

To be clear - I'm arguing in favour of signatures here. Measured boot
is much easier to work with in their presence.

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:

> Johannes made cfg80211 recently just use request_firmware() now via commit on
> linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
> he got tired of waiting firmware signing, but note he implemented a signature
> checking on its own so he open codes verify_pkcs7_signature() after the
> request_firmware() call. If we are happy to live with this, then so be it.
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0

Johannes was tired of waiting?  Commit 5a9196d "ima: add support for
measuring and appraising firmware" has been in the kernel since linux-
3.17.

The original firmware hook for verifying firmware signatures were
replaced with the common LSM pre and post kernel_read_file() hooks
in linux-4.6.y.

Even if you wanted to support firmware signature verification without
IMA-appraisal, it should be using the LSM hooks.

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Wed, Nov 15, 2017 at 06:49:57AM -0500, Mimi Zohar wrote:
> On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> 
> > Johannes made cfg80211 recently just use request_firmware() now via commit on
> > linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
> > he got tired of waiting firmware signing, but note he implemented a signature
> > checking on its own so he open codes verify_pkcs7_signature() after the
> > request_firmware() call. If we are happy to live with this, then so be it.
> > 
> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
> 
> Johannes was tired of waiting?  Commit 5a9196d "ima: add support for
> measuring and appraising firmware" has been in the kernel since linux-
> 3.17.
> 
> The original firmware hook for verifying firmware signatures were
> replaced with the common LSM pre and post kernel_read_file() hooks
> in linux-4.6.y.
> 
> Even if you wanted to support firmware signature verification without
> IMA-appraisal, it should be using the LSM hooks.

request_firmware() uses kernel_read_file_from_path() underneath the hood,
and so its used for both:

	/lib/firmware/regulatory.db
	/lib/firmware/regulatory.db.p7s

The later only if CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, which defaults
to y anyway.

What I meant was that net/wireless/reg.c now open codes firmware signature
validation on its own rather than using a helper. IMA appraisal will still
be used if enabled given kernel_read_file_from_path() is used.

The open coding of the firmware signature check is what I wanted to highlight.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Wed, 2017-11-15 at 18:52 +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 15, 2017 at 06:49:57AM -0500, Mimi Zohar wrote:
> > On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> > 
> > > Johannes made cfg80211 recently just use request_firmware() now via commit on
> > > linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
> > > he got tired of waiting firmware signing, but note he implemented a signature
> > > checking on its own so he open codes verify_pkcs7_signature() after the
> > > request_firmware() call. If we are happy to live with this, then so be it.
> > > 
> > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
> > 
> > Johannes was tired of waiting?  Commit 5a9196d "ima: add support for
> > measuring and appraising firmware" has been in the kernel since linux-
> > 3.17.
> > 
> > The original firmware hook for verifying firmware signatures were
> > replaced with the common LSM pre and post kernel_read_file() hooks
> > in linux-4.6.y.
> > 
> > Even if you wanted to support firmware signature verification without
> > IMA-appraisal, it should be using the LSM hooks.
> 
> request_firmware() uses kernel_read_file_from_path() underneath the hood,
> and so its used for both:
> 
> 	/lib/firmware/regulatory.db
> 	/lib/firmware/regulatory.db.p7s

The firmware signature validation should occur as part of
kernel_read_file_from_path(), not as a stand alone verification.

Why not extend kernel_read_file_from_path() to pass the detached signature?  Since the signature would only be used for the verification, there's no need to return the open file descriptor.

Or if you prefer, call kernel_read_file_from_path() for the detached signature first, and then call it again for the firmware with a pointer to the detached signature.

> 
> The later only if CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, which defaults
> to y anyway.
> 
> What I meant was that net/wireless/reg.c now open codes firmware signature
> validation on its own rather than using a helper. IMA appraisal will still
> be used if enabled given kernel_read_file_from_path() is used.
> 
> The open coding of the firmware signature check is what I wanted to highlight.

How are the keys in the CFG80211_EXTRA_REGDB_KEYDIR verified?  The
call to key_create_or_update() with the KEY_ALLOC_BYPASS_RESTRICTION
option by-passes any requirement that the keys in this directory are
signed.  This by-passes the concept of extending the secure boot
signature chain of trust.  To safely validate the keys use the
restrict_link_by_builtin_trusted option.

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Wed, Nov 15, 2017 at 02:56:57PM -0500, Mimi Zohar wrote:
> On Wed, 2017-11-15 at 18:52 +0100, Luis R. Rodriguez wrote:
> > On Wed, Nov 15, 2017 at 06:49:57AM -0500, Mimi Zohar wrote:
> > > On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> > > 
> > > > Johannes made cfg80211 recently just use request_firmware() now via commit on
> > > > linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
> > > > he got tired of waiting firmware signing, but note he implemented a signature
> > > > checking on its own so he open codes verify_pkcs7_signature() after the
> > > > request_firmware() call. If we are happy to live with this, then so be it.
> > > > 
> > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
> > > 
> > > Johannes was tired of waiting?  Commit 5a9196d "ima: add support for
> > > measuring and appraising firmware" has been in the kernel since linux-
> > > 3.17.
> > > 
> > > The original firmware hook for verifying firmware signatures were
> > > replaced with the common LSM pre and post kernel_read_file() hooks
> > > in linux-4.6.y.
> > > 
> > > Even if you wanted to support firmware signature verification without
> > > IMA-appraisal, it should be using the LSM hooks.
> > 
> > request_firmware() uses kernel_read_file_from_path() underneath the hood,
> > and so its used for both:
> > 
> > 	/lib/firmware/regulatory.db
> > 	/lib/firmware/regulatory.db.p7s
> 
> The firmware signature validation should occur as part of
> kernel_read_file_from_path(), not as a stand alone verification.
> 
> Why not extend kernel_read_file_from_path() to pass the detached signature?
> Since the signature would only be used for the verification, there's no need
> to return the open file descriptor.

This goes along with the question if there were an other users who wanted it,
or more importantly -- if firmware signing was desirable for any reason, a
modified kernel_read_file_from_path_signed() could in turn be used, *or* an LSM
added to handle READING_FIRMWARE and READING_FIRMWARE_PREALLOC_BUFFER.  The
above use case was one example outside of the typical firmware use.  I've long
pointed out that we no longer use the firmware API for just firmware, and the
above is now a very good example of it. I've been suggesting uses of the
firmware API for non-firmware had already happened and that more uses were on
its way. Trusted boot has nothing to do with these uses as such the gains of
systems pegged with "trusted boot" have nothing to do validation of these files
through hardware.

In this particular case its simply easier to re-use the existing redistribution
mechanisms in place already so that users can get the regulatory.db and
regulatory.db.p7s.

So here is now an example  use of request_firmware() API  with its own key
validation / keyring solution completely open coded.

So, given the wireless regulatory use case *does* benefit from re-using
request_firmware() due to the file redistribution aspects, using a new
kernel_read_file_from_path_signed() for non-firmware with the same
/lib/firmware/ path specified would be rather sloppy and awkward.  The right
way to not open code this is to consider adding firmware signing support
properly.

> > The later only if CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, which defaults
> > to y anyway.
> > 
> > What I meant was that net/wireless/reg.c now open codes firmware signature
> > validation on its own rather than using a helper. IMA appraisal will still
> > be used if enabled given kernel_read_file_from_path() is used.
> > 
> > The open coding of the firmware signature check is what I wanted to highlight.
> 
> How are the keys in the CFG80211_EXTRA_REGDB_KEYDIR verified?  The
> call to key_create_or_update() with the KEY_ALLOC_BYPASS_RESTRICTION
> option by-passes any requirement that the keys in this directory are
> signed.  This by-passes the concept of extending the secure boot
> signature chain of trust.  To safely validate the keys use the
> restrict_link_by_builtin_trusted option.

I'll let Johannes chime in, if he so wishes.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Wed, 2017-11-15 at 21:46 +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 15, 2017 at 02:56:57PM -0500, Mimi Zohar wrote:
> > On Wed, 2017-11-15 at 18:52 +0100, Luis R. Rodriguez wrote:
> > > On Wed, Nov 15, 2017 at 06:49:57AM -0500, Mimi Zohar wrote:
> > > > On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> > > > 
> > > > > Johannes made cfg80211 recently just use request_firmware() now via commit on
> > > > > linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
> > > > > he got tired of waiting firmware signing, but note he implemented a signature
> > > > > checking on its own so he open codes verify_pkcs7_signature() after the
> > > > > request_firmware() call. If we are happy to live with this, then so be it.
> > > > > 
> > > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
> > > > 
> > > > Johannes was tired of waiting?  Commit 5a9196d "ima: add support for
> > > > measuring and appraising firmware" has been in the kernel since linux-
> > > > 3.17.
> > > > 
> > > > The original firmware hook for verifying firmware signatures were
> > > > replaced with the common LSM pre and post kernel_read_file() hooks
> > > > in linux-4.6.y.
> > > > 
> > > > Even if you wanted to support firmware signature verification without
> > > > IMA-appraisal, it should be using the LSM hooks.
> > > 
> > > request_firmware() uses kernel_read_file_from_path() underneath the hood,
> > > and so its used for both:
> > > 
> > > 	/lib/firmware/regulatory.db
> > > 	/lib/firmware/regulatory.db.p7s
> > 
> > The firmware signature validation should occur as part of
> > kernel_read_file_from_path(), not as a stand alone verification.
> > 
> > Why not extend kernel_read_file_from_path() to pass the detached signature?
> > Since the signature would only be used for the verification, there's no need
> > to return the open file descriptor.
> 
> This goes along with the question if there were an other users who wanted it,
> or more importantly -- if firmware signing was desirable for any reason, a
> modified kernel_read_file_from_path_signed() could in turn be used, *or* an LSM
> added to handle READING_FIRMWARE and READING_FIRMWARE_PREALLOC_BUFFER.  The
> above use case was one example outside of the typical firmware use.  I've long
> pointed out that we no longer use the firmware API for just firmware, and the
> above is now a very good example of it. I've been suggesting uses of the
> firmware API for non-firmware had already happened and that more uses were on
> its way. Trusted boot has nothing to do with these uses as such the gains of
> systems pegged with "trusted boot" have nothing to do validation of these files
> through hardware.

No, it has nothing to do with other users wanting it.  It has to do
with extending an API to support detach signatures.

There's no reason to define a new function named
kernel_read_file_from_path_signed().  To prevent code duplication, the
existing functions would turn into wrappers.  It's not like there are
that many users.  A quick search returned:

kernel_read_file_from_fd:  2
kernel_read_file_from_path: 5
LSMs: 3 loadpin, selinux, + ima

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Matthew Garrett]

On Tue, Nov 14, 2017 at 3:18 PM, Linus Torvalds
<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> On Tue, Nov 14, 2017 at 11:58 AM, Matthew Garrett <mjg59-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> Our ability to determine that userland hasn't been tampered with
>> depends on the kernel being trustworthy. If userland can upload
>> arbitrary firmware to DMA-capable devices then we can no longer trust
>> the kernel. So yes, firmware is special.
>
> You're ignoring the whole "firmware is already signed by the hardware
> manufacturer and we don't even have access to it" part.

Firmware is sometimes signed by the hardware manufacturer. There's
plenty of hardware that accepts unsigned firmware.

> You're also ignoring the fact that we can't trust firmware _anyway_,
> as Alan pointed out.

Yeah, for arbitrary devices. There are cases where security has been
well audited, and it's viable to build systems where that's the
configuration you're running.

> Seriously. Some of the worst security issues have been with exactly
> the fact that we can't trust the hardware to begin with, where
> firmware/hardware combinations are not trusted to begin with.

You're right. But by that argument we might as well give up on *all*
security work - there's no way we can prove that a set of unprivileged
instructions won't backdoor a system.

> This is all theoretical security masturbation. The _real_ attacks have
> been elsewhere.

People made the same argument about Secure Boot, and then we
discovered that people *were* attacking the boot chain. As we secure
other components, the attackers move elsewhere. This is an attempt to
block off an avenue of attack before it's abused.

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Linus Torvalds]

On Tue, Nov 14, 2017 at 12:31 PM, Matthew Garrett <mjg59@google.com> wrote:
>
>> This is all theoretical security masturbation. The _real_ attacks have
>> been elsewhere.
>
> People made the same argument about Secure Boot, and then we
> discovered that people *were* attacking the boot chain. As we secure
> other components, the attackers move elsewhere. This is an attempt to
> block off an avenue of attack before it's abused.

The thing is, if you have attested the system from boot, then you've
already attested the firmware before it even gets loaded.

And if you haven't, then you can't trust anything else anyway.

So I really don't see your point.

             Linus

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Matthew Garrett]

On Tue, Nov 14, 2017 at 3:35 PM, Linus Torvalds
<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> On Tue, Nov 14, 2017 at 12:31 PM, Matthew Garrett <mjg59-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>
>>> This is all theoretical security masturbation. The _real_ attacks have
>>> been elsewhere.
>>
>> People made the same argument about Secure Boot, and then we
>> discovered that people *were* attacking the boot chain. As we secure
>> other components, the attackers move elsewhere. This is an attempt to
>> block off an avenue of attack before it's abused.
>
> The thing is, if you have attested the system from boot, then you've
> already attested the firmware before it even gets loaded.
>
> And if you haven't, then you can't trust anything else anyway.

Sure you can. You don't need attestation to be able to assert that the
system only booted signed code and that only you have control over
that signing key.

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

Hi!

> > Our ability to determine that userland hasn't been tampered with
> > depends on the kernel being trustworthy. If userland can upload
> > arbitrary firmware to DMA-capable devices then we can no longer trust
> > the kernel. So yes, firmware is special.
> 
> You're ignoring the whole "firmware is already signed by the hardware
> manufacturer and we don't even have access to it" part.

Well... I guess we'd prefer the firmware _not_ be signed, so we can
fix security holes in that after the vendor lost interest... Bugs in
the wifi stacks seemed patcheable that way.

There is GPLed firmware available for some USB wifi's. We really
should make sure firmware signing is not mandatory/encouraged for the hw vendors.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Tue, Dec 05, 2017 at 11:27:58AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Our ability to determine that userland hasn't been tampered with
> > > depends on the kernel being trustworthy. If userland can upload
> > > arbitrary firmware to DMA-capable devices then we can no longer trust
> > > the kernel. So yes, firmware is special.
> > 
> > You're ignoring the whole "firmware is already signed by the hardware
> > manufacturer and we don't even have access to it" part.
> 
> Well... I guess we'd prefer the firmware _not_ be signed, so we can
> fix security holes in that after the vendor lost interest... Bugs in
> the wifi stacks seemed patcheable that way.
> 
> There is GPLed firmware available for some USB wifi's. We really
> should make sure firmware signing is not mandatory/encouraged for the hw vendors.

I share this concern and interest, specially as more device drivers get purposely
stupid and most of the technology and fun things shift to firmware. Its precisely
for this same reason why most manufacturers won't be opening up more firmware, as
they could argue tons of value-add is in firmware now.

One has to be realistic though, as one of the few folks who pushed to open
source (and GPL) a few of the open source firmwares we have for WiFi, I realize
we have no option but to accept the fate that most manufacturers *will*
use and require signed firmware in the future.

If you're concerned about this you have only a few options.

One is to get more and more folks reverse engineer firmware. This won't help
if you can't deploy unsigned firmware though. But you have to also look at it
from a practical point of view, in order to do development you *have* to have
some sort of knobs to turn off fw signing verification, so it should be a
matter of looking hard. And for devices where this is not obvious or almost
impossible, hope for an exploit.

Another option is to argue for the engineering gain for having open firmware,
and a viable sensible and responsible option to continue to do R&D and innovation
with open firmware. We did this with WiFi long ago, see CFG80211_CERTIFICATION_ONUS
and the slew of open firmware released. The fact that we have open firmware
repositories and also a respective proprietary firmwares for the same device
could be used to metrically show the gains that such development had over time,
in a quantifiable form (features, bug fixes, security issues fixed). There's
tons of data available which could enable a researcher to have a field with
this.

And the last option is to just argue for a sensible opt-out knob to be documented
as part of our rights or due to general long term community security interests.
Just as with UEFI one can say one does not trust any key present already on the
platform, one should also be able to do the same for peripheral devices and their
own corresponding firmware -- a knob to disable firmware signing.

Sadly *all* of these are lofty pie in the sky objectives.

What the likely outcome will be is we sit and wait for the worst of the possible
security shit issues to hit the fan for the large IoT universe, and then hope
we can use this as leverage to require documenting such opt-out knobs just as
with UEFI's opt-out mechanism.

Although one would expect that such exploits already have happened, in practice
not at the interface we're talking about here, which is for /lib/firmware/.
For instance the Intel Management Engine does not use this interface, and
neither do some of the storage driver and arrays I've been bluntly told have
had tons of backdoors for years. Its precisely because of this lack of known
issues with security for /lib/firmware exploits that I'm willing to put signing
of /lib/firmware on the side for now.

We just haven't had many known attacks come in through here.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Alan Cox]

> One is to get more and more folks reverse engineer firmware. This won't help
> if you can't deploy unsigned firmware though. But you have to also look at it

Oh I think it will. From the experience with things like games consoles
most firmware is such a complete pile of s**t that you'll be able to
exploit it to load your own firmware through the bugs, and at the very
least after the 200th CVE against their firmware the vendor's direct
customers may be a bit fed up and demand proper process and visibility 8)

It's always going to be in tension though - because there is firmware you
really don't want people replacing maliciously, such as battery control,
or thermal protection.

Alan

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Thu, Nov 09, 2017 at 10:48:43AM +0900, AKASHI, Takahiro wrote:
> On Wed, Nov 08, 2017 at 08:46:26PM +0100, Luis R. Rodriguez wrote:
> > But perhaps I'm not understanding the issue well, let me know.
> 
> My point is quite simple:
> my_deviceA_init() {
>         err = request_firmware(&fw, "deviceA"); <--- (a)
>         if (err)
>                 goto err_request;
> 
>         err = verify_firmware(fw);  <--- (b)
>         if (err)
>                 goto err_verify;
> 
>         load_fw_to_deviceA(fw);     <--- (c)
>         ...
> }
> 
> As legacy device drivers does not have (b), there is no chance to
> prevent loading a firmware at (c) for locked-down kernel.

Ah, I think your example requires another piece of code to make it clearer.
Here is an example legacy driver:

my_legacy_deviceB_init() {
        err = request_firmware(&fw, "deviceB"); <--- (a)
        if (err)
                goto err_request;

        load_fw_to_deviceA(fw);     <--- (c)
        ...
}

There is no verify_firmware() call here, and as such the approach Linus
suggested a while ago cannot possibly fail on a "locked down kernel", unless
*very* legacy API call gets a verify_firmware() sprinkled.

One sensible thing to say here is then that all request_firmware() calls should
just fail on a "locked down kernel", however if this were true then even calls
which *did* issue a subsequent verify_firmware() would fail earlier therefore
making verify_firmware() pointless on new drivers.

Is that what you meant?

A long time ago we discussed similar default mechanisms, so its worth
re-iterating conclusions done before. The only difference with our current
discussion is that the latest proposed mechanism by Linus for firmware signing
was to consider a verify_firmware() so we can upkeep a functional API approach
to adding support for firmware signing.

What you are asking is what are default mechanism should be, and how do we
*ensure* this works using the functional API approach. We had ironed out
what the default mechanism should have been a while ago, not so much the
later as the firmware API has been in a state of flux. I'll provide pointers
to discussions around the default policy so that none of this is lost and
so that how we end up doing things is well understood later. We can try
to also hash out how to make this work then.

David Woodhouse has long recommended that a 'defult key' should only be last
resort [0], on that same email he also had suggested that since we could end up
supporting different signing schemes it may be best to support cryptographic
requirements (cert identifier or hash) from the start. James Bottomley has
recommended that the default behaviour can only be left up to "system policy".

A "system policy" is exactly what "kernel lockdown" is. David's email contained
a man page which stipulated that the policy that this type of system would follow
*is* to reject unsigned firmware. He and others may want to review if they indeed
wanted to follow this path in lieu of this email.

Even if one is not using a "kernel lockdown" system policy, we can consider the
old schemes discussed for firmware singing. In 2015 I first proposed a simple
signing scheme firmware signing which mimics the Linux module signing scheme
[2].  The issues with "purpose of a key" was well hashed out (a firmware key
should only work for firmware and only for the firmware it was designed for,
etc) and later iterations considered this thanks to Andy.

In this scheme, the default system policy depended on what kernel configuration
option your system had been built with, and matching the module signing scheme
we had:

	o CONFIG_FIRMWARE_SIG - "Firmware signature verification"
	o CONFIG_FIRMWARE_SIG_FORCE - "Require all firmware to be validly signed"

Everything about signing was done automatically behind the schemes for us, just
as with module signing.

If CONFIG_FIRMWARE_SIG_FORCE was *not* enabled, we'd grant unsigned firmwares
to be loaded, we'd just pr_warn() about them. Note that contrary to module
signing, firmware signing could not taint for technical reasons I listed on my
follow up patches after New Mexico Linux Plumbers [3]. Its worth re-iterating
so its not lost.

add_taint_module() is currently not exported, that can be fixed easily, but
also, the old firmware API currently does not associate the caller module in the
code, so we have a few options as to how to taint a kernel with unsigned firmware,
which I listed but I'll list here again:

  1) Extend the firmware_class driver API to require the correct module to
     always be set.

  2) Use add_taint_module() with the firmware_class module for the two
     synchrounous APIs, while using the right module for the async call.

  3) Use the firmware_class module for all 3 API calls when using
     add_taint_module()

  4) Skip tainting the kernel for unsigned firmware files

I went with 1) and 4) on that iteration of patches, with 1) only being done for
newer API calls. After this I stopped focusing on the firmware signing effort
as I first had to fix and clean up the firmware API in order to evolve it in a
more sane way for firmware signing later.

The "system policy" then was defined by kconfig. As per discussions it would
seem that some folks had issue with having a say the Linux Foundation being a
CA and say Kyle signing firmware would be kosher. In particular Alan Cox had
stated "A generic linux-firmware owned key would be both a horrendously
inviting attack target, and a single point of failure" [4].

Even if a default key is not used, old drivers may still desire to specify
a hash on an allowed firmware, and maybe that would suffice certain system
policies, however that is still a driver change, and an API call change, so
default policies for legacy drivers would still need to be considered when
evaluating and defining "system policies".

Andy has some work in progress worth evaluating for which could load a hash of
in-tree firmware into the kernel [5].

A system policy for "Kernel lockdown" may be compatible with the old approach
I had taken, its unclear and for that I'd ask David and others for feedback.

[0] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2015-July/001945.html
[1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2015-July/001961.html
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=fw-signing-v2-20150513
[3] https://lkml.kernel.org/r/1431541436-17007-1-git-send-email-mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org
[4] https://lkml.kernel.org/r/20150526180813.0ba1b5f5-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org
[5] https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=intree_module_hashes

> If you allow me to bring in yet another function, say
> request_firmware_signable(), which should be used in place of (a)
> for all verification-aware drivers, that would be fine.

Right, so you're suggesting that if we instead had firmware singing
implemented as a new call, that supporting different policies would be
not just much easier, but possible.

I see your point and indeed its unclear to me at this time how to make
verify_firmware() work for legacy drivers with a system policy that is
a strict "kernel lockdown" policy which rejects unsigned firmware.

The obvious alternative, which I had suggested in my first iteration of patches
was to do the signature check for every call underneath the hood for *all*
callers depending on a Kconfig symbols, just as we deal with module signing,
but that has the limitations listed above about taint and also requiring
a default key for legacy drivers. Even though some folks may have issues with
a default firmware key, it could solve that.

That would load foo.bin. and its signature file, say foo.bin.p7s or
foo.bin.sign, or whatever. So it could check a signature if one is required
(say kernel lock down system policy) and just fail.

> In this case, all the invocation of request_firmware() in legacy code
> could be forced to fail in locked-down kernel.

That's also another option and its worth to hash out.

Since we got into this because I noted I have given into the functional API
design approach I think its worth it to say a bit more about that in comparison
to the data driven API I had proposed a while ago and also a bit about LSMs.

If it can work (its unclear it seems yet), in *this** particular case I think
it can make some sense to ask of something like a new verify_firmware() call
for this feature, given it may imply to do a good amount of work, in this case
fetch a signature file and verify it.

It'd be kind of silly to add tons of _signable() variations, so provided we're
OK in a verify_firmware() call to load the signature *after*, I see no issue
here.

That said, long term the functional API approach still may require adding
variations for other minor features but which require less amount of work
and for which I think then I think its reasonable to say that a "radical
functional API" approach just makes no sense.

For example, today we have request_firmware() for sync and
request_firmware_direct() when we wish to make it explicit sync request is
optional. But we have no equivalent optional semantics for async calls, we may
need this in the future, and sprinkling a postfix just for that seems rather
silly to me, specially when the only difference is just a simple flag check. By
contrast a verify_firmware() call can do much more work, and as such it seems
fair to argue for it.

The reason I had given up with the data driven API was that it was also radical,
I had suggested only two API calls, a sync and an async call, and left one huge
data structure to enable to modify behaviour in any possible way. This is just
as radical.

Mandating a new API call per *any* new feature is just stupid as requiring a
full blown data structure for any conceivable feature for an API.

So a "radical functional API" design approach is just as crazy as a
   a "radical data driven API" design approach.

A middle ground is sensible.

The optional mechanism is one which clearly should just be a flag, and so
I'll be first fixing the firmware API flag mess.

Firmware signing however requires a different possible set of requirements so
depending on what is agreed upon for a default system policy for legacy drivers,
it may or may not make sense for a new separate API call.

Lastly, long ago it was discussed that how the Linux kernel does module
signing really should be an LSM, but back in the day we didn't have stackable
LSMs, so this was not possible. Since we do now support stackable LSMs,
moving module signing to an LSM is in theory possible, and how we handle
firmware signing could also be an LSM. This way the approach you take to
firmware signing is more tied down to a form of security system policy
rather than some random kernel config option.

> But I think that "signable" should be allowed to be combined with other
> features of request_firmware variants like _(no)wait or _direct.

Let's first hash out the default policy allowed and how it can work with verify_firmware()
or if we need to abandon that idea.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

On Fri, 2017-11-10 at 02:46 +0100, Luis R. Rodriguez wrote:
> On Thu, Nov 09, 2017 at 10:48:43AM +0900, AKASHI, Takahiro wrote:
> > On Wed, Nov 08, 2017 at 08:46:26PM +0100, Luis R. Rodriguez wrote:
> > > But perhaps I'm not understanding the issue well, let me know.
> > 
> > My point is quite simple:
> > my_deviceA_init() {
> >         err = request_firmware(&fw, "deviceA"); <--- (a)
> >         if (err)
> >                 goto err_request;
> > 
> >         err = verify_firmware(fw);  <--- (b)
> >         if (err)
> >                 goto err_verify;
> > 
> >         load_fw_to_deviceA(fw);     <--- (c)
> >         ...
> > }
> > 
> > As legacy device drivers does not have (b), there is no chance to
> > prevent loading a firmware at (c) for locked-down kernel.
> 
> Ah, I think your example requires another piece of code to make it clearer.
> Here is an example legacy driver:
> 
> my_legacy_deviceB_init() {
>         err = request_firmware(&fw, "deviceB"); <--- (a)
>         if (err)
>                 goto err_request;
> 
>         load_fw_to_deviceA(fw);     <--- (c)
>         ...
> }
> 
> There is no verify_firmware() call here, and as such the approach Linus
> suggested a while ago cannot possibly fail on a "locked down kernel", unless
> *very* legacy API call gets a verify_firmware() sprinkled.
> 
> One sensible thing to say here is then that all request_firmware() calls should
> just fail on a "locked down kernel", however if this were true then even calls
> which *did* issue a subsequent verify_firmware() would fail earlier therefore
> making verify_firmware() pointless on new drivers.

As long as these "*very* legacy API calls", are calling
kernel_read_file_from_path() to read the firmware, there shouldn't be
a problem.

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 08:45:06AM -0500, Mimi Zohar wrote:
> On Fri, 2017-11-10 at 02:46 +0100, Luis R. Rodriguez wrote:
> > On Thu, Nov 09, 2017 at 10:48:43AM +0900, AKASHI, Takahiro wrote:
> > > On Wed, Nov 08, 2017 at 08:46:26PM +0100, Luis R. Rodriguez wrote:
> > > > But perhaps I'm not understanding the issue well, let me know.
> > > 
> > > My point is quite simple:
> > > my_deviceA_init() {
> > >         err = request_firmware(&fw, "deviceA"); <--- (a)
> > >         if (err)
> > >                 goto err_request;
> > > 
> > >         err = verify_firmware(fw);  <--- (b)
> > >         if (err)
> > >                 goto err_verify;
> > > 
> > >         load_fw_to_deviceA(fw);     <--- (c)
> > >         ...
> > > }
> > > 
> > > As legacy device drivers does not have (b), there is no chance to
> > > prevent loading a firmware at (c) for locked-down kernel.
> > 
> > Ah, I think your example requires another piece of code to make it clearer.
> > Here is an example legacy driver:
> > 
> > my_legacy_deviceB_init() {
> >         err = request_firmware(&fw, "deviceB"); <--- (a)
> >         if (err)
> >                 goto err_request;
> > 
> >         load_fw_to_deviceA(fw);     <--- (c)
> >         ...
> > }
> > 
> > There is no verify_firmware() call here, and as such the approach Linus
> > suggested a while ago cannot possibly fail on a "locked down kernel", unless
> > *very* legacy API call gets a verify_firmware() sprinkled.
> > 
> > One sensible thing to say here is then that all request_firmware() calls should
> > just fail on a "locked down kernel", however if this were true then even calls
> > which *did* issue a subsequent verify_firmware() would fail earlier therefore
> > making verify_firmware() pointless on new drivers.
> 
> As long as these "*very* legacy API calls", are calling
> kernel_read_file_from_path() to read the firmware, there shouldn't be
> a problem.

For the *default* validation approach, agreed that we can LSMify this now through
kernel_read_file() by checking the enum kernel_read_file_id for READING_FIRMWARE,
as you did with your proposed LSM hook fw_lockdown_read_file(), but *iff* we
agree on which mechanisms to support for the default validation approach.

I say support given LSM'ifying this means anyone can end up trying to scheme up
a kernel solution which is non-IMA appraisals. For instance, based on
discussions so far, perhaps a sensible initial *default* scheme to strive for
firmware which we may not get a vendor to sign for us, or for which we don't
have a key to trust for singing could be to use a simple hash of files. This
makes sense for firmware which is typically not updated regularly and for which
we lost a source of updates for.

There's still a similar catch-22 issue here though:

A few folks have argued that we should support custom key requirements
from the start, so we can support a vendor wishing to use their own keys
from the start. Using a specific key is much more efficient for device
drivers which for instance may have firmware updated much more regularly,
this way a hash update on the kernel is not needed per firmware update.
Even if we support both schemes from the start, supporting a "default"
scheme without having any drivers update implies *all* firmware API calls
would need to fit the default policy scheme initially supported. So if
hashes of files is what would be used to start off with we'd be forcing all
firmware API kernel calls to follow that scheme from the start and I don't
think that makes much sense.

Despite some folks not liking it, supporting an initial default scheme just be
to be firmware signed by the linux-firmware maintainer might be an easier first
optional policy to support.

It does not mean we don't have to support hashes from the start, we can,
however that could require a driver change where its hash is specified or
preferred, for instance.

Likewise, for alternative and custom signing requirements, we also can require
a different API call, but for this to work well, I think we'd need a different
firmware API call and an LSM hook other than kernel_read_file_from_path() which
can enable the custom requirements to be specified. Otherwise we run into
similar issue as Takahiro pointed out with verify_firmware(), where the default
policy (say a default key) would not have a match from the start so it would
fail from the start. There is also the technical limitation that
kernel_read_file_from_path() has no way of letting callers pass further
security criteria.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Mon, Nov 13, 2017 at 07:50:35PM +0100, Luis R. Rodriguez wrote:
> On Fri, Nov 10, 2017 at 08:45:06AM -0500, Mimi Zohar wrote:
> It does not mean we don't have to support hashes from the start, we can,
> however that could require a driver change where its hash is specified or
> preferred, for instance.

Actually the pseudo code I just demo'd on your RFC proposal shows how we
could support the hashes for firmware an optional first policy and if that
fails check the fw signature if present. So no driver changes would be
needed other than key'ing a respective hash for the firmware, which can
just be a macro driver addition, not an API call change.

  Luis

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Mimi Zohar]

> > Or reflect that IMA-appraisal, if enabled, will enforce firmware being
> > validly signed.
> 
> But FWICT lockdown is a built-in kernel thingy, unless lockdown implies IMA
> it would not be the place to refer to it.
> 
> It seems the documentation was proposed to help users if an error was caught.
> That error should cover only what is being addressed in code on the kernel.

Enabling "lockdown" needs to take into account IMA-appraisal to
prevent breaking systems with it enabled.

An IMA builtin "secure_boot" policy was already upstreamed (commit
503ceaef8e2e "ima: define a set of appraisal rules requiring file
signatures").  An additional patch, automatically enables the
"secure_boot" policy in "lockdown" mode.

Refer to this discussion and patch:
http://kernsec.org/pipermail/linux-security-module-archive/2017-October/003913.html
http://kernsec.org/pipermail/linux-security-module-archive/2017-October/003910.html

thanks,

Mimi

Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

[Luis R. Rodriguez]

On Wed, Nov 08, 2017 at 03:01:09PM -0500, Mimi Zohar wrote:
> 
> > > Or reflect that IMA-appraisal, if enabled, will enforce firmware being
> > > validly signed.
> > 
> > But FWICT lockdown is a built-in kernel thingy, unless lockdown implies IMA
> > it would not be the place to refer to it.
> > 
> > It seems the documentation was proposed to help users if an error was caught.
> > That error should cover only what is being addressed in code on the kernel.
> 
> Enabling "lockdown" needs to take into account IMA-appraisal to
> prevent breaking systems with it enabled.
> 
> An IMA builtin "secure_boot" policy was already upstreamed (commit
> 503ceaef8e2e "ima: define a set of appraisal rules requiring file
> signatures").  An additional patch, automatically enables the
> "secure_boot" policy in "lockdown" mode.
> 
> Refer to this discussion and patch:
> http://kernsec.org/pipermail/linux-security-module-archive/2017-October/003913.html
> http://kernsec.org/pipermail/linux-security-module-archive/2017-October/003910.html

Ah then yeah this makes sense to mesh into the lock down documentation.

 Luis