2013-08-19 16:11:16

by Matthew Garrett

[permalink] [raw]
Subject: [PATCH 01/10] Add secure_modules() call

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 <[email protected]>
---
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


2013-08-19 16:11:12

by Matthew Garrett

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

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 <[email protected]>
---
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

2013-08-19 16:11:28

by Matthew Garrett

[permalink] [raw]
Subject: [PATCH 10/10] Add option to automatically enforce module signatures when in Secure Boot mode

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 <[email protected]>
---
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

2013-08-19 16:11:13

by Matthew Garrett

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

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 <[email protected]>
---
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

2013-08-19 16:11:49

by Matthew Garrett

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

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 <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
---
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

2013-08-19 16:12:40

by Matthew Garrett

[permalink] [raw]
Subject: [PATCH 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 <[email protected]>
---
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

2013-08-19 16:12:56

by Matthew Garrett

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

From: Josh Boyer <[email protected]>

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 <[email protected]>
---
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

2013-08-19 16:13:16

by Matthew Garrett

[permalink] [raw]
Subject: [PATCH 06/10] Restrict /dev/mem and /dev/kmem when module loading is restricted

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 <[email protected]>
---
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

2013-08-19 16:17:45

by Matthew Garrett

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

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 <[email protected]>
---
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..8105530 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 (!capable(CAP_COMPROMISE_KERNEL))
+ 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 (!capable(CAP_COMPROMISE_KERNEL))
+ return -EPERM;
+
status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
1, asus->debug.method_id,
&input, &output);
--
1.8.3.1

2013-08-19 16:17:59

by Matthew Garrett

[permalink] [raw]
Subject: [PATCH 04/10] ACPI: Limit access to custom_method

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 <[email protected]>
---
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

2013-08-19 16:20:20

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On Mon, Aug 19, 2013 at 9:10 AM, Matthew Garrett
<[email protected]> wrote:
> 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 <[email protected]>
> ---
> 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..8105530 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 (!capable(CAP_COMPROMISE_KERNEL))

Looks like this and the next chunk weren't changed to the
secure_modules() API...

-Kees

> + 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 (!capable(CAP_COMPROMISE_KERNEL))
> + return -EPERM;
> +
> status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID,
> 1, asus->debug.method_id,
> &input, &output);
> --
> 1.8.3.1
>



--
Kees Cook
Chrome OS Security

2013-08-19 17:15:43

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On Mon, 2013-08-19 at 09:20 -0700, Kees Cook wrote:

> Looks like this and the next chunk weren't changed to the
> secure_modules() API...

Bother. Yeah, looks like my test config had this left out.

--
Matthew Garrett <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2013-08-29 17:58:47

by Lenny Szubowicz

[permalink] [raw]
Subject: Re: [PATCH 04/10] ACPI: Limit access to custom_method


----- Original Message -----
> From: "Matthew Garrett" <[email protected]>
> To: [email protected]
> Cc: [email protected], [email protected], [email protected], "Matthew Garrett"
> <[email protected]>
> Sent: Monday, August 19, 2013 12:10:13 PM
> Subject: [PATCH 04/10] ACPI: Limit access to custom_method
>
> 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 <[email protected]>
> ---
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

Nit: Don't add a second include of linux/module.h.

-Lenny.

2013-08-29 18:22:42

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On 08/19/2013 09:10 AM, Matthew Garrett wrote:
> + if (!capable(CAP_COMPROMISE_KERNEL))
> + return -EPERM;
> +

Stale bits?

-hpa

2013-08-29 18:35:42

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On Thu, 2013-08-29 at 11:22 -0700, H. Peter Anvin wrote:
> On 08/19/2013 09:10 AM, Matthew Garrett wrote:
> > + if (!capable(CAP_COMPROMISE_KERNEL))
> > + return -EPERM;
> > +
>
> Stale bits?

Yeah. Did I manage to send out the old copy of that again? I'm sorry,
spending a few months concentrating on cloud stuff seems to have
entirely destroyed my ability to deal with git :(

--
Matthew Garrett <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2013-08-29 18:47:07

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On 08/29/2013 11:35 AM, Matthew Garrett wrote:
> On Thu, 2013-08-29 at 11:22 -0700, H. Peter Anvin wrote:
>> On 08/19/2013 09:10 AM, Matthew Garrett wrote:
>>> + if (!capable(CAP_COMPROMISE_KERNEL))
>>> + return -EPERM;
>>> +
>>
>> Stale bits?
>
> Yeah. Did I manage to send out the old copy of that again? I'm sorry,
> spending a few months concentrating on cloud stuff seems to have
> entirely destroyed my ability to deal with git :(
>

No, you mixed and matched in a single patch...

I still believe that CAP_RAWIO should be forbidden in this case.

-hpa

2013-08-29 18:49:18

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On Thu, 2013-08-29 at 11:46 -0700, H. Peter Anvin wrote:
> On 08/29/2013 11:35 AM, Matthew Garrett wrote:
> > On Thu, 2013-08-29 at 11:22 -0700, H. Peter Anvin wrote:
> >> On 08/19/2013 09:10 AM, Matthew Garrett wrote:
> >>> + if (!capable(CAP_COMPROMISE_KERNEL))
> >>> + return -EPERM;
> >>> +
> >>
> >> Stale bits?
> >
> > Yeah. Did I manage to send out the old copy of that again? I'm sorry,
> > spending a few months concentrating on cloud stuff seems to have
> > entirely destroyed my ability to deal with git :(
> >
>
> No, you mixed and matched in a single patch...

Right, but I'd fixed that in V2 (which I see I *did* send correctly, and
you're just replying to the old one :))

> I still believe that CAP_RAWIO should be forbidden in this case.

Seems fair.

--
Matthew Garrett <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2013-08-29 19:06:00

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On 08/29/2013 11:49 AM, Matthew Garrett wrote:
>>
>> No, you mixed and matched in a single patch...
>
> Right, but I'd fixed that in V2 (which I see I *did* send correctly, and
> you're just replying to the old one :))
>

Well, I'm responding to the one that was sent 31 minutes ago.

-hpa

2013-08-29 19:07:30

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On Thu, 2013-08-29 at 12:05 -0700, H. Peter Anvin wrote:
> On 08/29/2013 11:49 AM, Matthew Garrett wrote:
> >>
> >> No, you mixed and matched in a single patch...
> >
> > Right, but I'd fixed that in V2 (which I see I *did* send correctly, and
> > you're just replying to the old one :))
> >
>
> Well, I'm responding to the one that was sent 31 minutes ago.

Really? It's datestamped the 19th.

--
Matthew Garrett <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2013-08-29 19:28:19

by Josh Boyer

[permalink] [raw]
Subject: Re: [PATCH 05/10] asus-wmi: Restrict debugfs interface when module loading is restricted

On Thu, Aug 29, 2013 at 12:05:47PM -0700, H. Peter Anvin wrote:
> On 08/29/2013 11:49 AM, Matthew Garrett wrote:
> >>
> >> No, you mixed and matched in a single patch...
> >
> > Right, but I'd fixed that in V2 (which I see I *did* send correctly, and
> > you're just replying to the old one :))
> >
>
> Well, I'm responding to the one that was sent 31 minutes ago.

No patches were sent 31 minutes ago. And this is definitely in the
first series thread in my inbox.

josh

2013-09-01 08:52:48

by Geert Uytterhoeven

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

On Mon, Aug 19, 2013 at 6:10 PM, Matthew Garrett
<[email protected]> 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.

Any plans for signed kexec code?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds