[PATCH 0/10] Add additional security checks when module loading is restricted

[PATCH 0/10] Add additional security checks when module loading is restricted

[Matthew Garrett]

We have two in-kernel mechanisms for restricting module loading - disabling
it entirely, or limiting it to the loading of modules signed with a trusted
key. These can both be configured in such a way that even root is unable to
relax the restrictions.

However, right now, there's several other straightforward ways for root to
modify running kernel code. At the most basic level these allow root to
reset the configuration such that modules can be loaded again, rendering
the existing restrictions useless.

This patchset adds additional restrictions to various kernel entry points
that would otherwise make it straightforward for root to disable enforcement
of module loading restrictions. It also provides a patch that allows the
kernel to be configured such that module signing will be automatically
enabled when the system is booting via UEFI Secure Boot, allowing a stronger
guarantee of kernel integrity.

[PATCH V2 01/10] Add secure_modules() call

[Matthew Garrett]

Provide a single call to allow kernel code to determine whether the system
has been configured to either disable module loading entirely or to load
only modules signed with a trusted key.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 include/linux/module.h | 7 +++++++
 kernel/module.c        | 9 +++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index 46f1ea0..0c266b2 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -509,6 +509,8 @@ int unregister_module_notifier(struct notifier_block * nb);
 
 extern void print_modules(void);
 
+extern bool secure_modules(void);
+
 #else /* !CONFIG_MODULES... */
 
 /* Given an address, look for it in the exception tables. */
@@ -619,6 +621,11 @@ static inline int unregister_module_notifier(struct notifier_block * nb)
 static inline void print_modules(void)
 {
 }
+
+static inline bool secure_modules(void)
+{
+	return false;
+}
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
diff --git a/kernel/module.c b/kernel/module.c
index 2069158..801021e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3852,3 +3852,12 @@ void module_layout(struct module *mod,
 }
 EXPORT_SYMBOL(module_layout);
 #endif
+
+bool secure_modules(void)
+{
+#ifdef CONFIG_MODULE_SIG
+	return (sig_enforce || modules_disabled);
+#else
+	return modules_disabled;
+#endif
+}
-- 
1.8.3.1

[PATCH V2 02/10] PCI: Lock down BAR access when module security is enabled

[Matthew Garrett]

Any hardware that can potentially generate DMA has to be locked down from
userspace 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>
---
 drivers/pci/pci-sysfs.c | 10 ++++++++++
 drivers/pci/proc.c      |  8 +++++++-
 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 c0dbe1f..cd4e35f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/vgaarb.h>
 #include <linux/pm_runtime.h>
+#include <linux/module.h>
 #include "pci.h"
 
 static int sysfs_initialized;	/* = 0 */
@@ -624,6 +625,9 @@ pci_write_config(struct file* filp, struct kobject *kobj,
 	loff_t init_off = off;
 	u8 *data = (u8*) buf;
 
+	if (secure_modules())
+		return -EPERM;
+
 	if (off > dev->cfg_size)
 		return 0;
 	if (off + count > dev->cfg_size) {
@@ -930,6 +934,9 @@ pci_mmap_resource(struct kobject *kobj, struct bin_attribute *attr,
 	resource_size_t start, end;
 	int i;
 
+	if (secure_modules())
+		return -EPERM;
+
 	for (i = 0; i < PCI_ROM_RESOURCE; i++)
 		if (res == &pdev->resource[i])
 			break;
@@ -1037,6 +1044,9 @@ pci_write_resource_io(struct file *filp, struct kobject *kobj,
 		      struct bin_attribute *attr, char *buf,
 		      loff_t off, size_t count)
 {
+	if (secure_modules())
+		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 cdc7836..e3d498b 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -117,6 +117,9 @@ proc_bus_pci_write(struct file *file, const char __user *buf, size_t nbytes, lof
 	int size = dev->cfg_size;
 	int cnt;
 
+	if (secure_modules())
+		return -EPERM;
+
 	if (pos >= size)
 		return 0;
 	if (nbytes >= size)
@@ -196,6 +199,9 @@ static long proc_bus_pci_ioctl(struct file *file, unsigned int cmd,
 #endif /* HAVE_PCI_MMAP */
 	int ret = 0;
 
+	if (secure_modules())
+		return -EPERM;
+
 	switch (cmd) {
 	case PCIIOC_CONTROLLER:
 		ret = pci_domain_nr(dev->bus);
@@ -234,7 +240,7 @@ 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;
 
-	if (!capable(CAP_SYS_RAWIO))
+	if (!capable(CAP_SYS_RAWIO) || secure_modules())
 		return -EPERM;
 
 	/* Make sure the caller is mapping a real resource for this device */
diff --git a/drivers/pci/syscall.c b/drivers/pci/syscall.c
index e1c1ec5..bffbf71 100644
--- a/drivers/pci/syscall.c
+++ b/drivers/pci/syscall.c
@@ -10,6 +10,7 @@
 #include <linux/errno.h>
 #include <linux/pci.h>
 #include <linux/syscalls.h>
+#include <linux/module.h>
 #include <asm/uaccess.h>
 #include "pci.h"
 
@@ -92,7 +93,7 @@ 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) || secure_modules())
 		return -EPERM;
 
 	dev = pci_get_bus_and_slot(bus, dfn);
-- 
1.8.3.1

[PATCH V2 03/10] x86: Lock down IO port access when module security is enabled

[Matthew Garrett]

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.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 arch/x86/kernel/ioport.c | 5 +++--
 drivers/char/mem.c       | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/ioport.c b/arch/x86/kernel/ioport.c
index 4ddaf66..00b4403 100644
--- a/arch/x86/kernel/ioport.c
+++ b/arch/x86/kernel/ioport.c
@@ -15,6 +15,7 @@
 #include <linux/thread_info.h>
 #include <linux/syscalls.h>
 #include <linux/bitmap.h>
+#include <linux/module.h>
 #include <asm/syscalls.h>
 
 /*
@@ -28,7 +29,7 @@ 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) || secure_modules()))
 		return -EPERM;
 
 	/*
@@ -103,7 +104,7 @@ 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) || secure_modules())
 			return -EPERM;
 	}
 	regs->flags = (regs->flags & ~X86_EFLAGS_IOPL) | (level << 12);
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index f895a8c..1af8664 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -28,6 +28,7 @@
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/aio.h>
+#include <linux/module.h>
 
 #include <asm/uaccess.h>
 
@@ -563,6 +564,9 @@ static ssize_t write_port(struct file *file, const char __user *buf,
 	unsigned long i = *ppos;
 	const char __user *tmp = buf;
 
+	if (secure_modules())
+		return -EPERM;
+
 	if (!access_ok(VERIFY_READ, buf, count))
 		return -EFAULT;
 	while (count-- > 0 && i < 65536) {
-- 
1.8.3.1

[PATCH V2 04/10] ACPI: Limit access to custom_method

[Matthew Garrett]

custom_method effectively allows arbitrary access to system memory, making
it possible for an attacker to circumvent restrictions on module loading.
Disable it if any such restrictions have been enabled.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 drivers/acpi/custom_method.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/custom_method.c b/drivers/acpi/custom_method.c
index 12b62f2..55a013f 100644
--- a/drivers/acpi/custom_method.c
+++ b/drivers/acpi/custom_method.c
@@ -7,6 +7,7 @@
 #include <linux/kernel.h>
 #include <linux/uaccess.h>
 #include <linux/debugfs.h>
+#include <linux/module.h>
 #include <acpi/acpi_drivers.h>
 
 #include "internal.h"
@@ -29,6 +30,9 @@ static ssize_t cm_write(struct file *file, const char __user * user_buf,
 	struct acpi_table_header table;
 	acpi_status status;
 
+	if (secure_modules())
+		return -EPERM;
+
 	if (!(*ppos)) {
 		/* parse the table header to get the table length */
 		if (count <= sizeof(struct acpi_table_header))
-- 
1.8.3.1

[PATCH V2 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

[Matthew Garrett]

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 any of
these features are enabled.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 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 19c313b..db18ef66 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1618,6 +1618,9 @@ static int show_dsts(struct seq_file *m, void *data)
 	int err;
 	u32 retval = -1;
 
+	if (secure_modules())
+		return -EPERM;
+
 	err = asus_wmi_get_devstate(asus, asus->debug.dev_id, &retval);
 
 	if (err < 0)
@@ -1634,6 +1637,9 @@ static int show_devs(struct seq_file *m, void *data)
 	int err;
 	u32 retval = -1;
 
+	if (secure_modules())
+		return -EPERM;
+
 	err = asus_wmi_set_devstate(asus->debug.dev_id, asus->debug.ctrl_param,
 				    &retval);
 
@@ -1658,6 +1664,9 @@ static int show_call(struct seq_file *m, void *data)
 	union acpi_object *obj;
 	acpi_status status;
 
+	if (secure_modules())
+		return -EPERM;
+
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
 				     1, asus->debug.method_id,
 				     &input, &output);
-- 
1.8.3.1

[PATCH V2 06/10] Restrict /dev/mem and /dev/kmem when module loading is restricted

[Matthew Garrett]

Allowing users to write to address space makes it possible for the kernel
to be subverted, avoiding module loading restrictions. Prevent this when
any restrictions have been imposed on loading modules.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 drivers/char/mem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 1af8664..61406c8 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -159,6 +159,9 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
 	unsigned long copied;
 	void *ptr;
 
+	if (secure_modules())
+		return -EPERM;
+
 	if (!valid_phys_addr_range(p, count))
 		return -EFAULT;
 
@@ -497,6 +500,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 (secure_modules())
+		return -EPERM;
+
 	if (p < (unsigned long) high_memory) {
 		unsigned long to_write = min_t(unsigned long, count,
 					       (unsigned long)high_memory - p);
-- 
1.8.3.1

[PATCH V2 07/10] acpi: Ignore acpi_rsdp kernel parameter when module loading is restricted

[Matthew Garrett]

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 circumvent any restrictions imposed on
loading modules. Disable it in that case.

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 drivers/acpi/osl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..e4c4410 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -45,6 +45,7 @@
 #include <linux/list.h>
 #include <linux/jiffies.h>
 #include <linux/semaphore.h>
+#include <linux/module.h>
 
 #include <asm/io.h>
 #include <asm/uaccess.h>
@@ -245,7 +246,7 @@ early_param("acpi_rsdp", setup_acpi_rsdp);
 acpi_physical_address __init acpi_os_get_root_pointer(void)
 {
 #ifdef CONFIG_KEXEC
-	if (acpi_rsdp)
+	if (acpi_rsdp && !secure_modules())
 		return acpi_rsdp;
 #endif
 
-- 
1.8.3.1

[PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions

[Matthew Garrett]

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

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 kernel/kexec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index 59f7b55..1a7690f 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -32,6 +32,7 @@
 #include <linux/vmalloc.h>
 #include <linux/swap.h>
 #include <linux/syscore_ops.h>
+#include <linux/module.h>
 
 #include <asm/page.h>
 #include <asm/uaccess.h>
@@ -1645,6 +1646,9 @@ int kernel_kexec(void)
 		goto Unlock;
 	}
 
+	if (secure_modules())
+		return -EPERM;
+
 #ifdef CONFIG_KEXEC_JUMP
 	if (kexec_image->preserve_context) {
 		lock_system_sleep();
-- 
1.8.3.1

[PATCH V2 09/10] x86: Restrict MSR access when module loading is restricted

[Matthew Garrett]

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

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 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 88458fa..d08f7e3 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -103,6 +103,9 @@ static ssize_t msr_write(struct file *file, const char __user *buf,
 	int err = 0;
 	ssize_t bytes = 0;
 
+	if (secure_modules())
+		return -EPERM;
+
 	if (count % 8)
 		return -EINVAL;	/* Invalid chunk size */
 
@@ -150,6 +153,10 @@ static long msr_ioctl(struct file *file, unsigned int ioc, unsigned long arg)
 			err = -EBADF;
 			break;
 		}
+		if (secure_modules()) {
+			err = -EPERM;
+			break;
+		}
 		if (copy_from_user(&regs, uregs, sizeof regs)) {
 			err = -EFAULT;
 			break;
-- 
1.8.3.1

[PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

[Matthew Garrett]

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 enforces this automatically when enabled.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 Documentation/x86/zero-page.txt       |  2 ++
 arch/x86/Kconfig                      | 10 ++++++++++
 arch/x86/boot/compressed/eboot.c      | 33 +++++++++++++++++++++++++++++++++
 arch/x86/include/uapi/asm/bootparam.h |  3 ++-
 arch/x86/kernel/setup.c               |  6 ++++++
 include/linux/module.h                |  6 ++++++
 kernel/module.c                       |  7 +++++++
 7 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 199f453..ec38acf 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -30,6 +30,8 @@ Offset	Proto	Name		Meaning
 1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
 1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
 				(below)
+1EB/001	ALL     kbd_status      Numlock is enabled
+1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
 1EF/001	ALL	sentinel	Used to detect broken bootloaders
 290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
 2D0/A00	ALL	e820_map	E820 memory map table
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b32ebf9..6a6c19b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1581,6 +1581,16 @@ config EFI_STUB
 
 	  See Documentation/x86/efi-stub.txt for more information.
 
+config EFI_SECURE_BOOT_SIG_ENFORCE
+        def_bool n
+	prompt "Force module signing when UEFI Secure Boot is enabled"
+	---help---
+	  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.
+	  Say Y here to automatically enable module signature enforcement
+	  when a system boots with UEFI Secure Boot enabled.
+
 config SECCOMP
 	def_bool y
 	prompt "Enable seccomp to safely compute untrusted bytecode"
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..145294d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -861,6 +861,37 @@ fail:
 	return status;
 }
 
+static int get_secure_boot(efi_system_table_t *_table)
+{
+	u8 sb, setup;
+	unsigned long datasize = sizeof(sb);
+	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
+	efi_status_t status;
+
+	status = efi_call_phys5(sys_table->runtime->get_variable,
+				L"SecureBoot", &var_guid, NULL, &datasize, &sb);
+
+	if (status != EFI_SUCCESS)
+		return 0;
+
+	if (sb == 0)
+		return 0;
+
+
+	status = efi_call_phys5(sys_table->runtime->get_variable,
+				L"SetupMode", &var_guid, NULL, &datasize,
+				&setup);
+
+	if (status != EFI_SUCCESS)
+		return 0;
+
+	if (setup == 1)
+		return 0;
+
+	return 1;
+}
+
+
 /*
  * Because the x86 boot code expects to be passed a boot_params we
  * need to create one ourselves (usually the bootloader would create
@@ -1169,6 +1200,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
 		goto fail;
 
+	boot_params->secure_boot = get_secure_boot(sys_table);
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..d35da96 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -131,7 +131,8 @@ struct boot_params {
 	__u8  eddbuf_entries;				/* 0x1e9 */
 	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
 	__u8  kbd_status;				/* 0x1eb */
-	__u8  _pad5[3];					/* 0x1ec */
+	__u8  secure_boot;				/* 0x1ec */
+	__u8  _pad5[2];					/* 0x1ec */
 	/*
 	 * The sentinel is set to a nonzero value (0xff) in header.S.
 	 *
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f8ec578..deeb7bc 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1129,6 +1129,12 @@ void __init setup_arch(char **cmdline_p)
 
 	io_delay_init();
 
+#ifdef CONFIG_EFI_SECURE_BOOT_SIG_ENFORCE
+	if (boot_params.secure_boot) {
+		enforce_signed_modules();
+	}
+#endif
+
 	/*
 	 * Parse the ACPI tables for possible boot-time SMP configuration.
 	 */
diff --git a/include/linux/module.h b/include/linux/module.h
index 0c266b2..5a6374a 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -184,6 +184,12 @@ const struct exception_table_entry *search_exception_tables(unsigned long add);
 
 struct notifier_block;
 
+#ifdef CONFIG_MODULE_SIG
+extern void enforce_signed_modules(void);
+#else
+static inline void enforce_signed_modules(void) {};
+#endif
+
 #ifdef CONFIG_MODULES
 
 extern int modules_disabled; /* for sysctl */
diff --git a/kernel/module.c b/kernel/module.c
index 801021e..0ec29c0 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3853,6 +3853,13 @@ void module_layout(struct module *mod,
 EXPORT_SYMBOL(module_layout);
 #endif
 
+#ifdef CONFIG_MODULE_SIG
+void enforce_signed_modules(void)
+{
+	sig_enforce = true;
+}
+#endif
+
 bool secure_modules(void)
 {
 #ifdef CONFIG_MODULE_SIG
-- 
1.8.3.1

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[Kees Cook]

On Mon, Aug 19, 2013 at 10:26 AM, Matthew Garrett
<matthew.garrett@nebula.com> wrote:
> We have two in-kernel mechanisms for restricting module loading - disabling
> it entirely, or limiting it to the loading of modules signed with a trusted
> key. These can both be configured in such a way that even root is unable to
> relax the restrictions.
>
> However, right now, there's several other straightforward ways for root to
> modify running kernel code. At the most basic level these allow root to
> reset the configuration such that modules can be loaded again, rendering
> the existing restrictions useless.
>
> This patchset adds additional restrictions to various kernel entry points
> that would otherwise make it straightforward for root to disable enforcement
> of module loading restrictions. It also provides a patch that allows the
> kernel to be configured such that module signing will be automatically
> enabled when the system is booting via UEFI Secure Boot, allowing a stronger
> guarantee of kernel integrity.

I like this approach of attaching it to the module loading logic.
Given the LSM hook for loading modules, I think I'd like to add a hook
for the secure_modules() check so that an LSM can respond "secure" as
well, if it doing module loading mediation (for example, in the case
of Chrome OS's modules-only-from-rootfs). I'll work up a patch...

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[Lenny Szubowicz]

----- Original Message -----
> From: "Matthew Garrett" <matthew.garrett@nebula.com>
> To: linux-kernel@vger.kernel.org
> Cc: linux-efi@vger.kernel.org, jwboyer@redhat.com, keescook@chromium.org
> Sent: Monday, August 19, 2013 1:26:01 PM
> Subject: [PATCH 0/10] Add additional security checks when module loading is restricted
> 
> We have two in-kernel mechanisms for restricting module loading - disabling
> it entirely, or limiting it to the loading of modules signed with a trusted
> key. These can both be configured in such a way that even root is unable to
> relax the restrictions.
> 
> However, right now, there's several other straightforward ways for root to
> modify running kernel code. At the most basic level these allow root to
> reset the configuration such that modules can be loaded again, rendering
> the existing restrictions useless.
> 
> This patchset adds additional restrictions to various kernel entry points
> that would otherwise make it straightforward for root to disable enforcement
> of module loading restrictions. It also provides a patch that allows the
> kernel to be configured such that module signing will be automatically
> enabled when the system is booting via UEFI Secure Boot, allowing a stronger
> guarantee of kernel integrity.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Did you purposely exclude similar checks for hibernate that were covered
by earlier versions of your patch set?

I think you should include it and any patch set that attempts to add support
for doing hibernate securely would make appropriate changes.

                              -Lenny.

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 838 bytes --]

On Wed, 2013-08-28 at 18:37 -0400, Lenny Szubowicz wrote:

> Did you purposely exclude similar checks for hibernate that were covered
> by earlier versions of your patch set?

Yes, I think it's worth tying it in with the encrypted hibernation
support. The local attack is significantly harder in the hibernation
case - in the face of unknown hardware it basically involves a
pre-generated memory image corresponding to your system or the ability
to force a reboot into an untrusted environment. I think it's probably
more workable to just add a configuration option for forcing encrypted
hibernation when secure boot is in use.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[Lenny Szubowicz]

----- Original Message -----
> From: "Matthew Garrett" <matthew.garrett@nebula.com>
> To: "Lenny Szubowicz" <lszubowi@redhat.com>
> Cc: linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org, jwboyer@redhat.com, keescook@chromium.org
> Sent: Wednesday, August 28, 2013 6:41:55 PM
> Subject: Re: [PATCH 0/10] Add additional security checks when module loading is restricted
> 
> On Wed, 2013-08-28 at 18:37 -0400, Lenny Szubowicz wrote:
> 
> > Did you purposely exclude similar checks for hibernate that were covered
> > by earlier versions of your patch set?
> 
> Yes, I think it's worth tying it in with the encrypted hibernation
> support. The local attack is significantly harder in the hibernation
> case - in the face of unknown hardware it basically involves a
> pre-generated memory image corresponding to your system or the ability
> to force a reboot into an untrusted environment. I think it's probably
> more workable to just add a configuration option for forcing encrypted
> hibernation when secure boot is in use.
> 
> --
> Matthew Garrett <matthew.garrett@nebula.com>

I'm root. So I can write anything I want to the swap file that looks
like a valid hibernate image but is code of my choosing. I can read
anything I need from /dev/mem or /dev/kmem to help me do that.
I can then immediately initiate a reboot.

                               -Lenny.

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1558 bytes --]

On Wed, 2013-08-28 at 18:58 -0400, Lenny Szubowicz wrote:

> I'm root. So I can write anything I want to the swap file that looks
> like a valid hibernate image but is code of my choosing. I can read
> anything I need from /dev/mem or /dev/kmem to help me do that.
> I can then immediately initiate a reboot.

No, you're blocked from /dev/mem and /dev/kmem. That doesn't make it
impossible, but it does make it much harder. A more realistic attack is
to write something that looks like (but isn't) a hibernation image which
effectively jumps back into the resume kernel after modifying it, but
you'd still need to generate a bunch of kernel state.

The need for a reboot makes it a less significant attack than the others
that this patchset protects against, which all allow the modification of
the already running kernel. If you also want to protect against attacks
involving reboots then you need to secure the on-disk representation of
the kernel as well, which means Secure Boot, and that also means you
want encrypted hibernation support.

If you need something for the short term then I'd suggest just adding a
config option that disables hibernation when a system is in Secure Boot
mode, but the best plan is pretty much to review the encrypted
hibernation patches that got posted recently. It'd be easy to tie those
into appropriate policy.
-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[Kees Cook]

On Wed, Aug 28, 2013 at 3:58 PM, Lenny Szubowicz <lszubowi@redhat.com> wrote:
>
>
> ----- Original Message -----
>> From: "Matthew Garrett" <matthew.garrett@nebula.com>
>> To: "Lenny Szubowicz" <lszubowi@redhat.com>
>> Cc: linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org, jwboyer@redhat.com, keescook@chromium.org
>> Sent: Wednesday, August 28, 2013 6:41:55 PM
>> Subject: Re: [PATCH 0/10] Add additional security checks when module loading is restricted
>>
>> On Wed, 2013-08-28 at 18:37 -0400, Lenny Szubowicz wrote:
>>
>> > Did you purposely exclude similar checks for hibernate that were covered
>> > by earlier versions of your patch set?
>>
>> Yes, I think it's worth tying it in with the encrypted hibernation
>> support. The local attack is significantly harder in the hibernation
>> case - in the face of unknown hardware it basically involves a
>> pre-generated memory image corresponding to your system or the ability
>> to force a reboot into an untrusted environment. I think it's probably
>> more workable to just add a configuration option for forcing encrypted
>> hibernation when secure boot is in use.
>>
>> --
>> Matthew Garrett <matthew.garrett@nebula.com>
>
> I'm root. So I can write anything I want to the swap file that looks
> like a valid hibernate image but is code of my choosing. I can read
> anything I need from /dev/mem or /dev/kmem to help me do that.
> I can then immediately initiate a reboot.

Strictly speaking, RAM contents are not available via /dev/*mem, even
to root. However, you can request a suspend image be written, but to
not enter hibernation. Then modify the image, and request a resume
from it.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 655 bytes --]

On Wed, 2013-08-28 at 16:07 -0700, Kees Cook wrote:

> Strictly speaking, RAM contents are not available via /dev/*mem, even
> to root. However, you can request a suspend image be written, but to
> not enter hibernation. Then modify the image, and request a resume
> from it.

Is that true? Oh, hm - I guess you could do it with uswsusp. Ugh. Now I
wonder what else you can do with uswsusp. Yeah, ok, I think that case
certainly needs handling.

-- 
Matthew Garrett <matthew.garrett@nebula.com>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH V2 01/10] Add secure_modules() call

[Josh Boyer]

On Mon, Aug 19, 2013 at 01:26:02PM -0400, Matthew Garrett wrote:
> Provide a single call to allow kernel code to determine whether the system
> has been configured to either disable module loading entirely or to load
> only modules signed with a trusted key.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  include/linux/module.h | 7 +++++++
>  kernel/module.c        | 9 +++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 46f1ea0..0c266b2 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -509,6 +509,8 @@ int unregister_module_notifier(struct notifier_block * nb);
>  
>  extern void print_modules(void);
>  
> +extern bool secure_modules(void);
> +
>  #else /* !CONFIG_MODULES... */
>  
>  /* Given an address, look for it in the exception tables. */
> @@ -619,6 +621,11 @@ static inline int unregister_module_notifier(struct notifier_block * nb)
>  static inline void print_modules(void)
>  {
>  }
> +
> +static inline bool secure_modules(void)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_MODULES */
>  
>  #ifdef CONFIG_SYSFS
> diff --git a/kernel/module.c b/kernel/module.c
> index 2069158..801021e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3852,3 +3852,12 @@ void module_layout(struct module *mod,
>  }
>  EXPORT_SYMBOL(module_layout);
>  #endif
> +
> +bool secure_modules(void)
> +{
> +#ifdef CONFIG_MODULE_SIG
> +	return (sig_enforce || modules_disabled);
> +#else
> +	return modules_disabled;
> +#endif
> +}

Needs an EXPORT_SYMBOL/EXPORT_SYMBOL_GPL here, or the patches to drivers
later in the series will fail to link.

josh

Re: [PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions

[Lenny Szubowicz]

----- Original Message -----
> From: "Matthew Garrett" <matthew.garrett@nebula.com>
> To: linux-kernel@vger.kernel.org
> Cc: linux-efi@vger.kernel.org, jwboyer@redhat.com, keescook@chromium.org, "Matthew Garrett"
> <matthew.garrett@nebula.com>
> Sent: Monday, August 19, 2013 1:26:09 PM
> Subject: [PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions
> 
> kexec permits the loading and execution of arbitrary code in ring 0, which
> is something that module signing enforcement is meant to prevent. It makes
> sense to disable kexec in this situation.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  kernel/kexec.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 59f7b55..1a7690f 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -32,6 +32,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/swap.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/module.h>
>  
>  #include <asm/page.h>
>  #include <asm/uaccess.h>
> @@ -1645,6 +1646,9 @@ int kernel_kexec(void)
>  		goto Unlock;
>  	}
>  
> +	if (secure_modules())
> +		return -EPERM;
> +
>  #ifdef CONFIG_KEXEC_JUMP
>  	if (kexec_image->preserve_context) {
>  		lock_system_sleep();
> --
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


Shouldn't that be:

+	if (secure_modules()) {
+		error = -EPERM;
+		goto Unlock;
+	}

-Lenny.

Re: [PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions

[Vivek Goyal]

On Mon, Aug 19, 2013 at 01:26:09PM -0400, Matthew Garrett wrote:
> kexec permits the loading and execution of arbitrary code in ring 0, which
> is something that module signing enforcement is meant to prevent. It makes
> sense to disable kexec in this situation.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  kernel/kexec.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 59f7b55..1a7690f 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -32,6 +32,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/swap.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/module.h>
>  
>  #include <asm/page.h>
>  #include <asm/uaccess.h>
> @@ -1645,6 +1646,9 @@ int kernel_kexec(void)
>  		goto Unlock;
>  	}
>  
> +	if (secure_modules())
> +		return -EPERM;
> +

Hi Matthew, 

Why do disallow this at run time. To me denying this at kexec_load() time
sounds better. Otherwise a user will think that I loaded crash kernel
successfully and when crash actually happens, we never capture the
crash dump.

Thanks
Vivek

Re: [PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions

[Lenny Szubowicz]

----- Original Message -----
> From: "Lenny Szubowicz" <lszubowi@redhat.com>
> To: "Matthew Garrett" <matthew.garrett@nebula.com>, linux-kernel@vger.kernel.org
> Cc: linux-efi@vger.kernel.org, jwboyer@redhat.com, keescook@chromium.org
> Sent: Thursday, August 29, 2013 11:57:36 AM
> Subject: Re: [PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces module loading restrictions
> 
> 
> 
> ----- Original Message -----
> > From: "Matthew Garrett" <matthew.garrett@nebula.com>
> > To: linux-kernel@vger.kernel.org
> > Cc: linux-efi@vger.kernel.org, jwboyer@redhat.com, keescook@chromium.org,
> > "Matthew Garrett"
> > <matthew.garrett@nebula.com>
> > Sent: Monday, August 19, 2013 1:26:09 PM
> > Subject: [PATCH V2 08/10] kexec: Disable at runtime if the kernel enforces
> > module loading restrictions
> > 
> > kexec permits the loading and execution of arbitrary code in ring 0, which
> > is something that module signing enforcement is meant to prevent. It makes
> > sense to disable kexec in this situation.
> > 
> > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> > ---
> >  kernel/kexec.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > index 59f7b55..1a7690f 100644
> > --- a/kernel/kexec.c
> > +++ b/kernel/kexec.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/vmalloc.h>
> >  #include <linux/swap.h>
> >  #include <linux/syscore_ops.h>
> > +#include <linux/module.h>
> >  
> >  #include <asm/page.h>
> >  #include <asm/uaccess.h>
> > @@ -1645,6 +1646,9 @@ int kernel_kexec(void)
> >  		goto Unlock;
> >  	}
> >  
> > +	if (secure_modules())
> > +		return -EPERM;
> > +
> >  #ifdef CONFIG_KEXEC_JUMP
> >  	if (kexec_image->preserve_context) {
> >  		lock_system_sleep();
> > --
> > 1.8.3.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> 
> 
> Shouldn't that be:
> 
> +	if (secure_modules()) {
> +		error = -EPERM;
> +		goto Unlock;
> +	}
> 
> -Lenny.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Also, in earlier incarnations you disallowed kexec load in secure mode.
But now you allow the load. You then disallow the transfer of control
to the loaded code via kernel_kexec in secure mode.

Even if there might be no risk by having untrusted code remain loaded, why
not prevent it up front?

                            -Lenny.

Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

[Josh Boyer]

On Mon, Aug 19, 2013 at 01:26:11PM -0400, Matthew Garrett wrote:
> UEFI Secure Boot provides a mechanism for ensuring that the firmware will
> only load signed bootloaders and kernels. Certain use cases may also
> require that all kernel modules also be signed. Add a configuration option
> that enforces this automatically when enabled.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  Documentation/x86/zero-page.txt       |  2 ++
>  arch/x86/Kconfig                      | 10 ++++++++++
>  arch/x86/boot/compressed/eboot.c      | 33 +++++++++++++++++++++++++++++++++
>  arch/x86/include/uapi/asm/bootparam.h |  3 ++-
>  arch/x86/kernel/setup.c               |  6 ++++++
>  include/linux/module.h                |  6 ++++++
>  kernel/module.c                       |  7 +++++++
>  7 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
> index 199f453..ec38acf 100644
> --- a/Documentation/x86/zero-page.txt
> +++ b/Documentation/x86/zero-page.txt
> @@ -30,6 +30,8 @@ Offset	Proto	Name		Meaning
>  1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
>  1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
>  				(below)
> +1EB/001	ALL     kbd_status      Numlock is enabled
> +1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
>  1EF/001	ALL	sentinel	Used to detect broken bootloaders
>  290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
>  2D0/A00	ALL	e820_map	E820 memory map table
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b32ebf9..6a6c19b 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1581,6 +1581,16 @@ config EFI_STUB
>  
>  	  See Documentation/x86/efi-stub.txt for more information.
>  
> +config EFI_SECURE_BOOT_SIG_ENFORCE
> +        def_bool n
> +	prompt "Force module signing when UEFI Secure Boot is enabled"
> +	---help---
> +	  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.
> +	  Say Y here to automatically enable module signature enforcement
> +	  when a system boots with UEFI Secure Boot enabled.
> +
>  config SECCOMP
>  	def_bool y
>  	prompt "Enable seccomp to safely compute untrusted bytecode"
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index b7388a4..145294d 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -861,6 +861,37 @@ fail:
>  	return status;
>  }
>  
> +static int get_secure_boot(efi_system_table_t *_table)
> +{
> +	u8 sb, setup;
> +	unsigned long datasize = sizeof(sb);
> +	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
> +	efi_status_t status;
> +
> +	status = efi_call_phys5(sys_table->runtime->get_variable,
> +				L"SecureBoot", &var_guid, NULL, &datasize, &sb);
> +
> +	if (status != EFI_SUCCESS)
> +		return 0;
> +
> +	if (sb == 0)
> +		return 0;
> +
> +
> +	status = efi_call_phys5(sys_table->runtime->get_variable,
> +				L"SetupMode", &var_guid, NULL, &datasize,
> +				&setup);
> +
> +	if (status != EFI_SUCCESS)
> +		return 0;
> +
> +	if (setup == 1)
> +		return 0;
> +
> +	return 1;
> +}
> +
> +
>  /*
>   * Because the x86 boot code expects to be passed a boot_params we
>   * need to create one ourselves (usually the bootloader would create
> @@ -1169,6 +1200,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
>  	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
>  		goto fail;
>  
> +	boot_params->secure_boot = get_secure_boot(sys_table);
> +
>  	setup_graphics(boot_params);
>  
>  	setup_efi_pci(boot_params);
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index c15ddaf..d35da96 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -131,7 +131,8 @@ struct boot_params {
>  	__u8  eddbuf_entries;				/* 0x1e9 */
>  	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
>  	__u8  kbd_status;				/* 0x1eb */
> -	__u8  _pad5[3];					/* 0x1ec */
> +	__u8  secure_boot;				/* 0x1ec */
> +	__u8  _pad5[2];					/* 0x1ec */
>  	/*
>  	 * The sentinel is set to a nonzero value (0xff) in header.S.
>  	 *

You need to include the following chunk of code with this, otherwise the
secure_boot variable gets cleared.

diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 4a8cb8d..25f9cf1 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -38,9 +38,13 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 		memset(&boot_params->ext_ramdisk_image, 0,
 		       (char *)&boot_params->efi_info -
 			(char *)&boot_params->ext_ramdisk_image);
-		memset(&boot_params->kbd_status, 0,
+		memset(&boot_params->kbd_status, 0, sizeof(boot_params->kbd_status));
+		/* don't clear boot_params->secure_boot.  we set that ourselves
+		 * earlier.
+		 */
+		memset(&boot_params->_pad5[0], 0,
 		       (char *)&boot_params->hdr -
-		       (char *)&boot_params->kbd_status);
+		       (char *)&boot_params->_pad5[0]);
 		memset(&boot_params->_pad7[0], 0,
 		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
 			(char *)&boot_params->_pad7[0]);


josh

Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

[H. Peter Anvin]

On 08/29/2013 11:37 AM, Josh Boyer wrote:
>>  	setup_efi_pci(boot_params);
>> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
>> index c15ddaf..d35da96 100644
>> --- a/arch/x86/include/uapi/asm/bootparam.h
>> +++ b/arch/x86/include/uapi/asm/bootparam.h
>> @@ -131,7 +131,8 @@ struct boot_params {
>>  	__u8  eddbuf_entries;				/* 0x1e9 */
>>  	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
>>  	__u8  kbd_status;				/* 0x1eb */
>> -	__u8  _pad5[3];					/* 0x1ec */
>> +	__u8  secure_boot;				/* 0x1ec */
>> +	__u8  _pad5[2];					/* 0x1ec */
>>  	/*
>>  	 * The sentinel is set to a nonzero value (0xff) in header.S.
>>  	 *
> 
> You need to include the following chunk of code with this, otherwise the
> secure_boot variable gets cleared.
> 

Not really.

There are three cases:

1. Boot stub only.  Here we do the right thing with the bootparams.
2. Boot loader bypasses the boot stub completely.  Here we MUST NOT do
   what you suggest above.
3. Boot stub with a boot_params structure passed in.  Here we should
   run sanitize_boot_params() (an inline for a reason) in the boot
   stub, before we set the secure_boot field.  Once that is done, we
   again don't need that modification.

	-hpa

Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

[Josh Boyer]

On Fri, Aug 30, 2013 at 01:46:30PM -0700, H. Peter Anvin wrote:
> On 08/29/2013 11:37 AM, Josh Boyer wrote:
> >>  	setup_efi_pci(boot_params);
> >> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> >> index c15ddaf..d35da96 100644
> >> --- a/arch/x86/include/uapi/asm/bootparam.h
> >> +++ b/arch/x86/include/uapi/asm/bootparam.h
> >> @@ -131,7 +131,8 @@ struct boot_params {
> >>  	__u8  eddbuf_entries;				/* 0x1e9 */
> >>  	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
> >>  	__u8  kbd_status;				/* 0x1eb */
> >> -	__u8  _pad5[3];					/* 0x1ec */
> >> +	__u8  secure_boot;				/* 0x1ec */
> >> +	__u8  _pad5[2];					/* 0x1ec */
> >>  	/*
> >>  	 * The sentinel is set to a nonzero value (0xff) in header.S.
> >>  	 *
> > 
> > You need to include the following chunk of code with this, otherwise the
> > secure_boot variable gets cleared.
> > 
> 
> Not really.
> 
> There are three cases:
> 
> 1. Boot stub only.  Here we do the right thing with the bootparams.
> 2. Boot loader bypasses the boot stub completely.  Here we MUST NOT do
>    what you suggest above.
> 3. Boot stub with a boot_params structure passed in.  Here we should
>    run sanitize_boot_params() (an inline for a reason) in the boot
>    stub, before we set the secure_boot field.  Once that is done, we
>    again don't need that modification.

OK.  If 3 works, then great.  All I know is that Fedora has been
carrying the above hunk for months and it was missing in this patch set.
So when I went to test it, the patches didn't do anything because the
secure_boot field was getting cleared.

I'm more than happy to try option 3, and I'll poke at it next week
unless someone beats me to it.

josh

Re: [PATCH 0/10] Add additional security checks when module loading is restricted

[joeyli]

於 三，2013-08-28 於 16:07 -0700，Kees Cook 提到：
> On Wed, Aug 28, 2013 at 3:58 PM, Lenny Szubowicz <lszubowi@redhat.com> wrote:
> >
> >
> > ----- Original Message -----
> >> From: "Matthew Garrett" <matthew.garrett@nebula.com>
> >> To: "Lenny Szubowicz" <lszubowi@redhat.com>
> >> Cc: linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org, jwboyer@redhat.com, keescook@chromium.org
> >> Sent: Wednesday, August 28, 2013 6:41:55 PM
> >> Subject: Re: [PATCH 0/10] Add additional security checks when module loading is restricted
> >>
> >> On Wed, 2013-08-28 at 18:37 -0400, Lenny Szubowicz wrote:
> >>
> >> > Did you purposely exclude similar checks for hibernate that were covered
> >> > by earlier versions of your patch set?
> >>
> >> Yes, I think it's worth tying it in with the encrypted hibernation
> >> support. The local attack is significantly harder in the hibernation
> >> case - in the face of unknown hardware it basically involves a
> >> pre-generated memory image corresponding to your system or the ability
> >> to force a reboot into an untrusted environment. I think it's probably
> >> more workable to just add a configuration option for forcing encrypted
> >> hibernation when secure boot is in use.
> >>
> >> --
> >> Matthew Garrett <matthew.garrett@nebula.com>
> >
> > I'm root. So I can write anything I want to the swap file that looks
> > like a valid hibernate image but is code of my choosing. I can read
> > anything I need from /dev/mem or /dev/kmem to help me do that.
> > I can then immediately initiate a reboot.
> 
> Strictly speaking, RAM contents are not available via /dev/*mem, even
> to root. However, you can request a suspend image be written, but to
> not enter hibernation. Then modify the image, and request a resume
> from it.
> 
> -Kees
> 

I agreed!

As a userland hibernate tool, it possible trigger kernel to generate a
snapshot image of current memory, read the snapshot, modify and upload
it back to the temporary memory space of snapshot, trigger S4 resume to
restore it.

The signature check of S4 snapshot can prevent this attack because the
patches put the signature of snapshot image to snapshot header. Even
attacker change the signature of header or modified the data page in
snapshot. The modified snapshot image will not pass by signature check.


Thanks a lot!
Joey Lee

Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

[joeyli]

於 五，2013-08-30 於 19:41 -0400，Josh Boyer 提到：
> On Fri, Aug 30, 2013 at 01:46:30PM -0700, H. Peter Anvin wrote:
> > On 08/29/2013 11:37 AM, Josh Boyer wrote:
> > >>  	setup_efi_pci(boot_params);
> > >> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> > >> index c15ddaf..d35da96 100644
> > >> --- a/arch/x86/include/uapi/asm/bootparam.h
> > >> +++ b/arch/x86/include/uapi/asm/bootparam.h
> > >> @@ -131,7 +131,8 @@ struct boot_params {
> > >>  	__u8  eddbuf_entries;				/* 0x1e9 */
> > >>  	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
> > >>  	__u8  kbd_status;				/* 0x1eb */
> > >> -	__u8  _pad5[3];					/* 0x1ec */
> > >> +	__u8  secure_boot;				/* 0x1ec */
> > >> +	__u8  _pad5[2];					/* 0x1ec */
> > >>  	/*
> > >>  	 * The sentinel is set to a nonzero value (0xff) in header.S.
> > >>  	 *
> > > 
> > > You need to include the following chunk of code with this, otherwise the
> > > secure_boot variable gets cleared.
> > > 
> > 
> > Not really.
> > 
> > There are three cases:
> > 
> > 1. Boot stub only.  Here we do the right thing with the bootparams.
> > 2. Boot loader bypasses the boot stub completely.  Here we MUST NOT do
> >    what you suggest above.
> > 3. Boot stub with a boot_params structure passed in.  Here we should
> >    run sanitize_boot_params() (an inline for a reason) in the boot
> >    stub, before we set the secure_boot field.  Once that is done, we
> >    again don't need that modification.
> 
> OK.  If 3 works, then great.  All I know is that Fedora has been
> carrying the above hunk for months and it was missing in this patch set.
> So when I went to test it, the patches didn't do anything because the
> secure_boot field was getting cleared.
> 
> I'm more than happy to try option 3, and I'll poke at it next week
> unless someone beats me to it.
> 
> josh

The secure_boot field cleaned by sanitize_boot_params() when using grub2
linuxefi to load efi stub kernel.
I printed the boot_params->sentinel value, confirm this value is NOT 0
when running grub2 linuxefi path, the entry point is efi_stub_entry. 

On the other hand,
the sentinel value is 0 when direct run efi stub kernel in UEFI shell,
the secure_boot field can keep.

Does that mean grub2 should clean the sentinel value? or we move the get
secure_boot value to efi_init()?


Thanks a lot!
Joey Lee

Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

[Josh Boyer]

On Wed, Sep 4, 2013 at 6:51 AM, joeyli <jlee@suse.com> wrote:
> 於 五，2013-08-30 於 19:41 -0400，Josh Boyer 提到：
>> On Fri, Aug 30, 2013 at 01:46:30PM -0700, H. Peter Anvin wrote:
>> > On 08/29/2013 11:37 AM, Josh Boyer wrote:
>> > >>          setup_efi_pci(boot_params);
>> > >> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
>> > >> index c15ddaf..d35da96 100644
>> > >> --- a/arch/x86/include/uapi/asm/bootparam.h
>> > >> +++ b/arch/x86/include/uapi/asm/bootparam.h
>> > >> @@ -131,7 +131,8 @@ struct boot_params {
>> > >>          __u8  eddbuf_entries;                           /* 0x1e9 */
>> > >>          __u8  edd_mbr_sig_buf_entries;                  /* 0x1ea */
>> > >>          __u8  kbd_status;                               /* 0x1eb */
>> > >> -        __u8  _pad5[3];                                 /* 0x1ec */
>> > >> +        __u8  secure_boot;                              /* 0x1ec */
>> > >> +        __u8  _pad5[2];                                 /* 0x1ec */
>> > >>          /*
>> > >>           * The sentinel is set to a nonzero value (0xff) in header.S.
>> > >>           *
>> > >
>> > > You need to include the following chunk of code with this, otherwise the
>> > > secure_boot variable gets cleared.
>> > >
>> >
>> > Not really.
>> >
>> > There are three cases:
>> >
>> > 1. Boot stub only.  Here we do the right thing with the bootparams.
>> > 2. Boot loader bypasses the boot stub completely.  Here we MUST NOT do
>> >    what you suggest above.
>> > 3. Boot stub with a boot_params structure passed in.  Here we should
>> >    run sanitize_boot_params() (an inline for a reason) in the boot
>> >    stub, before we set the secure_boot field.  Once that is done, we
>> >    again don't need that modification.
>>
>> OK.  If 3 works, then great.  All I know is that Fedora has been
>> carrying the above hunk for months and it was missing in this patch set.
>> So when I went to test it, the patches didn't do anything because the
>> secure_boot field was getting cleared.
>>
>> I'm more than happy to try option 3, and I'll poke at it next week
>> unless someone beats me to it.
>>
>> josh
>
> The secure_boot field cleaned by sanitize_boot_params() when using grub2
> linuxefi to load efi stub kernel.
> I printed the boot_params->sentinel value, confirm this value is NOT 0
> when running grub2 linuxefi path, the entry point is efi_stub_entry.
>
> On the other hand,
> the sentinel value is 0 when direct run efi stub kernel in UEFI shell,
> the secure_boot field can keep.
>
> Does that mean grub2 should clean the sentinel value? or we move the get
> secure_boot value to efi_init()?

See V3 of this patch that Matthew sent yesterday.  It calls
sanitize_boot_params in efi_main before calling get_secure_boot.  I
tested that yesterday and it worked fine.

josh

Re: [PATCH V2 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

[joeyli]

於 三，2013-09-04 於 08:01 -0400，Josh Boyer 提到：
> On Wed, Sep 4, 2013 at 6:51 AM, joeyli <jlee@suse.com> wrote:
> > 於 五，2013-08-30 於 19:41 -0400，Josh Boyer 提到：
> >> On Fri, Aug 30, 2013 at 01:46:30PM -0700, H. Peter Anvin wrote:
> >> > On 08/29/2013 11:37 AM, Josh Boyer wrote:
> >> > >>          setup_efi_pci(boot_params);
> >> > >> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> >> > >> index c15ddaf..d35da96 100644
> >> > >> --- a/arch/x86/include/uapi/asm/bootparam.h
> >> > >> +++ b/arch/x86/include/uapi/asm/bootparam.h
> >> > >> @@ -131,7 +131,8 @@ struct boot_params {
> >> > >>          __u8  eddbuf_entries;                           /* 0x1e9 */
> >> > >>          __u8  edd_mbr_sig_buf_entries;                  /* 0x1ea */
> >> > >>          __u8  kbd_status;                               /* 0x1eb */
> >> > >> -        __u8  _pad5[3];                                 /* 0x1ec */
> >> > >> +        __u8  secure_boot;                              /* 0x1ec */
> >> > >> +        __u8  _pad5[2];                                 /* 0x1ec */
> >> > >>          /*
> >> > >>           * The sentinel is set to a nonzero value (0xff) in header.S.
> >> > >>           *
> >> > >
> >> > > You need to include the following chunk of code with this, otherwise the
> >> > > secure_boot variable gets cleared.
> >> > >
> >> >
> >> > Not really.
> >> >
> >> > There are three cases:
> >> >
> >> > 1. Boot stub only.  Here we do the right thing with the bootparams.
> >> > 2. Boot loader bypasses the boot stub completely.  Here we MUST NOT do
> >> >    what you suggest above.
> >> > 3. Boot stub with a boot_params structure passed in.  Here we should
> >> >    run sanitize_boot_params() (an inline for a reason) in the boot
> >> >    stub, before we set the secure_boot field.  Once that is done, we
> >> >    again don't need that modification.
> >>
> >> OK.  If 3 works, then great.  All I know is that Fedora has been
> >> carrying the above hunk for months and it was missing in this patch set.
> >> So when I went to test it, the patches didn't do anything because the
> >> secure_boot field was getting cleared.
> >>
> >> I'm more than happy to try option 3, and I'll poke at it next week
> >> unless someone beats me to it.
> >>
> >> josh
> >
> > The secure_boot field cleaned by sanitize_boot_params() when using grub2
> > linuxefi to load efi stub kernel.
> > I printed the boot_params->sentinel value, confirm this value is NOT 0
> > when running grub2 linuxefi path, the entry point is efi_stub_entry.
> >
> > On the other hand,
> > the sentinel value is 0 when direct run efi stub kernel in UEFI shell,
> > the secure_boot field can keep.
> >
> > Does that mean grub2 should clean the sentinel value? or we move the get
> > secure_boot value to efi_init()?
> 
> See V3 of this patch that Matthew sent yesterday.  It calls
> sanitize_boot_params in efi_main before calling get_secure_boot.  I
> tested that yesterday and it worked fine.
> 
> josh

Ah!
Thanks for you point out, I missed his v3 patch.

Joey Lee