2012-10-04 20:23:09

by Kees Cook

[permalink] [raw]
Subject: [PATCH v5] module: add syscall to load module from fd

Hi,

This is a rebase onto Rusty's module-next tree. The syscall number
additions show the expected changes that are living in linux-next already,
just to avoid horrible collisions there.

I would _really_ like this to get into the 3.7 window, if possible. It's
gotten lots of support, and I think its cleanups actually improve the
readability of the module code.

This that be pulled from my tree:
git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tracking-modules-next-finit_module

Thanks,

-Kees


2012-10-04 20:23:20

by Kees Cook

[permalink] [raw]
Subject: [PATCH 4/4] add finit_module syscall to asm-generic

This adds the finit_module syscall to the generic syscall list.

Signed-off-by: Kees Cook <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
---
include/asm-generic/unistd.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/unistd.h b/include/asm-generic/unistd.h
index 991ef01..7ef8507 100644
--- a/include/asm-generic/unistd.h
+++ b/include/asm-generic/unistd.h
@@ -691,9 +691,13 @@ __SC_COMP(__NR_process_vm_readv, sys_process_vm_readv, \
#define __NR_process_vm_writev 271
__SC_COMP(__NR_process_vm_writev, sys_process_vm_writev, \
compat_sys_process_vm_writev)
+#define __NR_kcmp 272
+__SYSCALL(__NR_kcmp, sys_kcmp)
+#define __NR_finit_module 273
+__SYSCALL(__NR_finit_module, sys_finit_module)

#undef __NR_syscalls
-#define __NR_syscalls 272
+#define __NR_syscalls 274

/*
* All syscalls below here should go away really,
--
1.7.9.5

2012-10-04 20:23:32

by Kees Cook

[permalink] [raw]
Subject: [PATCH 3/4] ARM: add finit_module syscall to ARM

Add finit_module syscall to the ARM syscall list.

Signed-off-by: Kees Cook <[email protected]>
Cc: Russell King <[email protected]>
---
arch/arm/include/asm/unistd.h | 2 ++
arch/arm/kernel/calls.S | 2 ++
2 files changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index 0cab47d..7959ac2 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -404,6 +404,8 @@
#define __NR_setns (__NR_SYSCALL_BASE+375)
#define __NR_process_vm_readv (__NR_SYSCALL_BASE+376)
#define __NR_process_vm_writev (__NR_SYSCALL_BASE+377)
+ /* 378 for kcmp */
+#define __NR_finit_module (__NR_SYSCALL_BASE+379)

/*
* The following SWIs are ARM private.
diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S
index 463ff4a..afce06a 100644
--- a/arch/arm/kernel/calls.S
+++ b/arch/arm/kernel/calls.S
@@ -387,6 +387,8 @@
/* 375 */ CALL(sys_setns)
CALL(sys_process_vm_readv)
CALL(sys_process_vm_writev)
+ CALL(sys_ni_syscall) /* reserved for sys_kcmp */
+ CALL(sys_finit_module)
#ifndef syscalls_counted
.equ syscalls_padding, ((NR_syscalls + 3) & ~3) - NR_syscalls
#define syscalls_counted
--
1.7.9.5

2012-10-04 20:23:40

by Kees Cook

[permalink] [raw]
Subject: [PATCH 1/4] module: add syscall to load module from fd

As part of the effort to create a stronger boundary between root and
kernel, Chrome OS wants to be able to enforce that kernel modules are
being loaded only from our read-only crypto-hash verified (dm_verity)
root filesystem. Since the init_module syscall hands the kernel a module
as a memory blob, no reasoning about the origin of the blob can be made.

Earlier proposals for appending signatures to kernel modules would not be
useful in Chrome OS, since it would involve adding an additional set of
keys to our kernel and builds for no good reason: we already trust the
contents of our root filesystem. We don't need to verify those kernel
modules a second time. Having to do signature checking on module loading
would slow us down and be redundant. All we need to know is where a
module is coming from so we can say yes/no to loading it.

If a file descriptor is used as the source of a kernel module, many more
things can be reasoned about. In Chrome OS's case, we could enforce that
the module lives on the filesystem we expect it to live on. In the case
of IMA (or other LSMs), it would be possible, for example, to examine
extended attributes that may contain signatures over the contents of
the module.

This introduces a new syscall (on x86), similar to init_module, that has
only two arguments. The first argument is used as a file descriptor to
the module and the second argument is a pointer to the NULL terminated
string of module arguments.

Signed-off-by: Kees Cook <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: Andrew Morton <[email protected]>
---
v5:
- rebase onto rusty's module-next tree.
v4:
- fixed misplaced ack (moved from ARM to asm-generic).
v3:
- fix ret from copy_from_user, thanks to Fengguang Wu.
- rename to finit_module, suggested by Peter Anvin.
- various cleanups, suggested from Andrew Morton.
v2:
- various cleanups, thanks to Rusty Russell.
---
arch/x86/syscalls/syscall_32.tbl | 1 +
arch/x86/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 1 +
kernel/module.c | 363 +++++++++++++++++++++++---------------
kernel/sys_ni.c | 1 +
5 files changed, 222 insertions(+), 145 deletions(-)

diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 7a35a6e..74ccf44 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -356,3 +356,4 @@
347 i386 process_vm_readv sys_process_vm_readv compat_sys_process_vm_readv
348 i386 process_vm_writev sys_process_vm_writev compat_sys_process_vm_writev
349 i386 kcmp sys_kcmp
+350 i386 finit_module sys_finit_module
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index a582bfe..7c58c84 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -319,6 +319,7 @@
310 64 process_vm_readv sys_process_vm_readv
311 64 process_vm_writev sys_process_vm_writev
312 common kcmp sys_kcmp
+313 common finit_module sys_finit_module

#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 19439c7..a377796 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -860,4 +860,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,

asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
unsigned long idx1, unsigned long idx2);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs);
#endif
diff --git a/kernel/module.c b/kernel/module.c
index 0e2da86..321c6b9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -21,6 +21,7 @@
#include <linux/ftrace_event.h>
#include <linux/init.h>
#include <linux/kallsyms.h>
+#include <linux/file.h>
#include <linux/fs.h>
#include <linux/sysfs.h>
#include <linux/kernel.h>
@@ -2420,12 +2421,12 @@ static inline void kmemleak_load_module(const struct module *mod,
#endif

#ifdef CONFIG_MODULE_SIG
-static int module_sig_check(struct load_info *info,
- const void *mod, unsigned long *len)
+static int module_sig_check(struct load_info *info)
{
int err = -ENOKEY;
const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
- const void *p = mod, *end = mod + *len;
+ const void *mod = info->hdr;
+ const void *p = mod, *end = mod + info->len;

/* Poor man's memmem. */
while ((p = memchr(p, MODULE_SIG_STRING[0], end - p))) {
@@ -2435,8 +2436,9 @@ static int module_sig_check(struct load_info *info,
if (memcmp(p, MODULE_SIG_STRING, markerlen) == 0) {
const void *sig = p + markerlen;
/* Truncate module up to signature. */
- *len = p - mod;
- err = mod_verify_sig(mod, *len, sig, end - sig);
+ info->len = p - mod;
+ err = mod_verify_sig(mod, info->len,
+ sig, end - sig);
break;
}
p++;
@@ -2457,59 +2459,97 @@ static int module_sig_check(struct load_info *info,
return err;
}
#else /* !CONFIG_MODULE_SIG */
-static int module_sig_check(struct load_info *info,
- void *mod, unsigned long *len)
+static int module_sig_check(struct load_info *info)
{
return 0;
}
#endif /* !CONFIG_MODULE_SIG */

-/* Sets info->hdr, info->len and info->sig_ok. */
-static int copy_and_check(struct load_info *info,
- const void __user *umod, unsigned long len,
- const char __user *uargs)
+/* Sanity checks against invalid binaries, wrong arch, weird elf version. */
+static int elf_header_check(struct load_info *info)
{
- int err;
- Elf_Ehdr *hdr;
+ if (info->len < sizeof(*(info->hdr)))
+ return -ENOEXEC;
+
+ if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
+ || info->hdr->e_type != ET_REL
+ || !elf_check_arch(info->hdr)
+ || info->hdr->e_shentsize != sizeof(Elf_Shdr))
+ return -ENOEXEC;
+
+ if (info->hdr->e_shoff >= info->len
+ || (info->hdr->e_shnum * sizeof(Elf_Shdr) >
+ info->len - info->hdr->e_shoff))
+ return -ENOEXEC;

- if (len < sizeof(*hdr))
+ return 0;
+}
+
+/* Sets info->hdr and info->len. */
+static int copy_module_from_user(const void __user *umod, unsigned long len,
+ struct load_info *info)
+{
+ info->len = len;
+ if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;

/* Suck in entire file: we'll want most of it. */
- if ((hdr = vmalloc(len)) == NULL)
+ info->hdr = vmalloc(info->len);
+ if (!info->hdr)
return -ENOMEM;

- if (copy_from_user(hdr, umod, len) != 0) {
- err = -EFAULT;
- goto free_hdr;
+ if (copy_from_user(info->hdr, umod, info->len) != 0) {
+ vfree(info->hdr);
+ return -EFAULT;
}

- err = module_sig_check(info, hdr, &len);
+ return 0;
+}
+
+/* Sets info->hdr and info->len. */
+static int copy_module_from_fd(int fd, struct load_info *info)
+{
+ struct file *file;
+ int err;
+ struct kstat stat;
+ loff_t pos;
+ ssize_t bytes = 0;
+
+ file = fget(fd);
+ if (!file)
+ return -ENOEXEC;
+
+ err = vfs_getattr(file->f_vfsmnt, file->f_dentry, &stat);
if (err)
- goto free_hdr;
+ goto out;

- /* Sanity checks against insmoding binaries or wrong arch,
- weird elf version */
- if (memcmp(hdr->e_ident, ELFMAG, SELFMAG) != 0
- || hdr->e_type != ET_REL
- || !elf_check_arch(hdr)
- || hdr->e_shentsize != sizeof(Elf_Shdr)) {
- err = -ENOEXEC;
- goto free_hdr;
+ if (stat.size > INT_MAX) {
+ err = -EFBIG;
+ goto out;
}
-
- if (hdr->e_shoff >= len ||
- hdr->e_shnum * sizeof(Elf_Shdr) > len - hdr->e_shoff) {
- err = -ENOEXEC;
- goto free_hdr;
+ info->hdr = vmalloc(stat.size);
+ if (!info->hdr) {
+ err = -ENOMEM;
+ goto out;
}

- info->hdr = hdr;
- info->len = len;
- return 0;
+ pos = 0;
+ while (pos < stat.size) {
+ bytes = kernel_read(file, pos, (char *)(info->hdr) + pos,
+ stat.size - pos);
+ if (bytes < 0) {
+ vfree(info->hdr);
+ err = bytes;
+ goto out;
+ }
+ if (bytes == 0)
+ break;
+ pos += bytes;
+ }
+ info->len = pos;

-free_hdr:
- vfree(hdr);
+out:
+ fput(file);
return err;
}

@@ -2948,33 +2988,123 @@ static bool finished_loading(const char *name)
return ret;
}

+/* Call module constructors. */
+static void do_mod_ctors(struct module *mod)
+{
+#ifdef CONFIG_CONSTRUCTORS
+ unsigned long i;
+
+ for (i = 0; i < mod->num_ctors; i++)
+ mod->ctors[i]();
+#endif
+}
+
+/* This is where the real work happens */
+static int do_init_module(struct module *mod)
+{
+ int ret = 0;
+
+ blocking_notifier_call_chain(&module_notify_list,
+ MODULE_STATE_COMING, mod);
+
+ /* Set RO and NX regions for core */
+ set_section_ro_nx(mod->module_core,
+ mod->core_text_size,
+ mod->core_ro_size,
+ mod->core_size);
+
+ /* Set RO and NX regions for init */
+ set_section_ro_nx(mod->module_init,
+ mod->init_text_size,
+ mod->init_ro_size,
+ mod->init_size);
+
+ do_mod_ctors(mod);
+ /* Start the module */
+ if (mod->init != NULL)
+ ret = do_one_initcall(mod->init);
+ if (ret < 0) {
+ /* Init routine failed: abort. Try to protect us from
+ buggy refcounters. */
+ mod->state = MODULE_STATE_GOING;
+ synchronize_sched();
+ module_put(mod);
+ blocking_notifier_call_chain(&module_notify_list,
+ MODULE_STATE_GOING, mod);
+ free_module(mod);
+ wake_up_all(&module_wq);
+ return ret;
+ }
+ if (ret > 0) {
+ printk(KERN_WARNING
+"%s: '%s'->init suspiciously returned %d, it should follow 0/-E convention\n"
+"%s: loading module anyway...\n",
+ __func__, mod->name, ret,
+ __func__);
+ dump_stack();
+ }
+
+ /* Now it's a first class citizen! */
+ mod->state = MODULE_STATE_LIVE;
+ blocking_notifier_call_chain(&module_notify_list,
+ MODULE_STATE_LIVE, mod);
+
+ /* We need to finish all async code before the module init sequence is done */
+ async_synchronize_full();
+
+ mutex_lock(&module_mutex);
+ /* Drop initial reference. */
+ module_put(mod);
+ trim_init_extable(mod);
+#ifdef CONFIG_KALLSYMS
+ mod->num_symtab = mod->core_num_syms;
+ mod->symtab = mod->core_symtab;
+ mod->strtab = mod->core_strtab;
+#endif
+ unset_module_init_ro_nx(mod);
+ module_free(mod, mod->module_init);
+ mod->module_init = NULL;
+ mod->init_size = 0;
+ mod->init_ro_size = 0;
+ mod->init_text_size = 0;
+ mutex_unlock(&module_mutex);
+ wake_up_all(&module_wq);
+
+ return 0;
+}
+
+static int may_init_module(void)
+{
+ if (!capable(CAP_SYS_MODULE) || modules_disabled)
+ return -EPERM;
+
+ return 0;
+}
+
/* Allocate and load the module: note that size of section 0 is always
zero, and we rely on this for optional sections. */
-static struct module *load_module(void __user *umod,
- unsigned long len,
- const char __user *uargs)
+static int load_module(struct load_info *info, const char __user *uargs)
{
- struct load_info info = { NULL, };
struct module *mod, *old;
long err;

- pr_debug("load_module: umod=%p, len=%lu, uargs=%p\n",
- umod, len, uargs);
+ err = module_sig_check(info);
+ if (err)
+ goto free_copy;

- /* Copy in the blobs from userspace, check they are vaguely sane. */
- err = copy_and_check(&info, umod, len, uargs);
+ err = elf_header_check(info);
if (err)
- return ERR_PTR(err);
+ goto free_copy;

/* Figure out module layout, and allocate all the memory. */
- mod = layout_and_allocate(&info);
+ mod = layout_and_allocate(info);
if (IS_ERR(mod)) {
err = PTR_ERR(mod);
goto free_copy;
}

#ifdef CONFIG_MODULE_SIG
- mod->sig_ok = info.sig_ok;
+ mod->sig_ok = info->sig_ok;
if (!mod->sig_ok)
add_taint_module(mod, TAINT_FORCED_MODULE);
#endif
@@ -2986,25 +3116,25 @@ static struct module *load_module(void __user *umod,

/* Now we've got everything in the final locations, we can
* find optional sections. */
- find_module_sections(mod, &info);
+ find_module_sections(mod, info);

err = check_module_license_and_versions(mod);
if (err)
goto free_unload;

/* Set up MODINFO_ATTR fields */
- setup_modinfo(mod, &info);
+ setup_modinfo(mod, info);

/* Fix up syms, so that st_value is a pointer to location. */
- err = simplify_symbols(mod, &info);
+ err = simplify_symbols(mod, info);
if (err < 0)
goto free_modinfo;

- err = apply_relocations(mod, &info);
+ err = apply_relocations(mod, info);
if (err < 0)
goto free_modinfo;

- err = post_relocation(mod, &info);
+ err = post_relocation(mod, info);
if (err < 0)
goto free_modinfo;

@@ -3044,14 +3174,14 @@ again:
}

/* This has to be done once we're sure module name is unique. */
- dynamic_debug_setup(info.debug, info.num_debug);
+ dynamic_debug_setup(info->debug, info->num_debug);

/* Find duplicate symbols */
err = verify_export_symbols(mod);
if (err < 0)
goto ddebug;

- module_bug_finalize(info.hdr, info.sechdrs, mod);
+ module_bug_finalize(info->hdr, info->sechdrs, mod);
list_add_rcu(&mod->list, &modules);
mutex_unlock(&module_mutex);

@@ -3062,16 +3192,17 @@ again:
goto unlink;

/* Link in to syfs. */
- err = mod_sysfs_setup(mod, &info, mod->kp, mod->num_kp);
+ err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
if (err < 0)
goto unlink;

/* Get rid of temporary copy. */
- free_copy(&info);
+ free_copy(info);

/* Done! */
trace_module_load(mod);
- return mod;
+
+ return do_init_module(mod);

unlink:
mutex_lock(&module_mutex);
@@ -3080,7 +3211,7 @@ again:
module_bug_cleanup(mod);
wake_up_all(&module_wq);
ddebug:
- dynamic_debug_remove(info.debug);
+ dynamic_debug_remove(info->debug);
unlock:
mutex_unlock(&module_mutex);
synchronize_sched();
@@ -3092,106 +3223,48 @@ again:
free_unload:
module_unload_free(mod);
free_module:
- module_deallocate(mod, &info);
+ module_deallocate(mod, info);
free_copy:
- free_copy(&info);
- return ERR_PTR(err);
-}
-
-/* Call module constructors. */
-static void do_mod_ctors(struct module *mod)
-{
-#ifdef CONFIG_CONSTRUCTORS
- unsigned long i;
-
- for (i = 0; i < mod->num_ctors; i++)
- mod->ctors[i]();
-#endif
+ free_copy(info);
+ return err;
}

-/* This is where the real work happens */
SYSCALL_DEFINE3(init_module, void __user *, umod,
unsigned long, len, const char __user *, uargs)
{
- struct module *mod;
- int ret = 0;
-
- /* Must have permission */
- if (!capable(CAP_SYS_MODULE) || modules_disabled)
- return -EPERM;
+ int err;
+ struct load_info info = { };

- /* Do all the hard work */
- mod = load_module(umod, len, uargs);
- if (IS_ERR(mod))
- return PTR_ERR(mod);
+ err = may_init_module();
+ if (err)
+ return err;

- blocking_notifier_call_chain(&module_notify_list,
- MODULE_STATE_COMING, mod);
+ pr_debug("init_module: umod=%p, len=%lu, uargs=%p\n",
+ umod, len, uargs);

- /* Set RO and NX regions for core */
- set_section_ro_nx(mod->module_core,
- mod->core_text_size,
- mod->core_ro_size,
- mod->core_size);
+ err = copy_module_from_user(umod, len, &info);
+ if (err)
+ return err;

- /* Set RO and NX regions for init */
- set_section_ro_nx(mod->module_init,
- mod->init_text_size,
- mod->init_ro_size,
- mod->init_size);
+ return load_module(&info, uargs);
+}

- do_mod_ctors(mod);
- /* Start the module */
- if (mod->init != NULL)
- ret = do_one_initcall(mod->init);
- if (ret < 0) {
- /* Init routine failed: abort. Try to protect us from
- buggy refcounters. */
- mod->state = MODULE_STATE_GOING;
- synchronize_sched();
- module_put(mod);
- blocking_notifier_call_chain(&module_notify_list,
- MODULE_STATE_GOING, mod);
- free_module(mod);
- wake_up_all(&module_wq);
- return ret;
- }
- if (ret > 0) {
- printk(KERN_WARNING
-"%s: '%s'->init suspiciously returned %d, it should follow 0/-E convention\n"
-"%s: loading module anyway...\n",
- __func__, mod->name, ret,
- __func__);
- dump_stack();
- }
+SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
+{
+ int err;
+ struct load_info info = { };

- /* Now it's a first class citizen! */
- mod->state = MODULE_STATE_LIVE;
- blocking_notifier_call_chain(&module_notify_list,
- MODULE_STATE_LIVE, mod);
+ err = may_init_module();
+ if (err)
+ return err;

- /* We need to finish all async code before the module init sequence is done */
- async_synchronize_full();
+ pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);

- mutex_lock(&module_mutex);
- /* Drop initial reference. */
- module_put(mod);
- trim_init_extable(mod);
-#ifdef CONFIG_KALLSYMS
- mod->num_symtab = mod->core_num_syms;
- mod->symtab = mod->core_symtab;
- mod->strtab = mod->core_strtab;
-#endif
- unset_module_init_ro_nx(mod);
- module_free(mod, mod->module_init);
- mod->module_init = NULL;
- mod->init_size = 0;
- mod->init_ro_size = 0;
- mod->init_text_size = 0;
- mutex_unlock(&module_mutex);
- wake_up_all(&module_wq);
+ err = copy_module_from_fd(fd, &info);
+ if (err)
+ return err;

- return 0;
+ return load_module(&info, uargs);
}

static inline int within(unsigned long addr, void *start, unsigned long size)
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index dbff751..395084d 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -25,6 +25,7 @@ cond_syscall(sys_swapoff);
cond_syscall(sys_kexec_load);
cond_syscall(compat_sys_kexec_load);
cond_syscall(sys_init_module);
+cond_syscall(sys_finit_module);
cond_syscall(sys_delete_module);
cond_syscall(sys_socketpair);
cond_syscall(sys_bind);
--
1.7.9.5

2012-10-04 20:24:11

by Kees Cook

[permalink] [raw]
Subject: [PATCH 2/4] security: introduce kernel_module_from_file hook

Now that kernel module origins can be reasoned about, provide a hook to
the LSMs to make policy decisions about the module file. This will let
Chrome OS enforce that loadable kernel modules can only come from its
read-only hash-verified root filesystem. Other LSMs can, for example,
read extended attributes for signatures, etc.

Signed-off-by: Kees Cook <[email protected]>
Acked-by: Serge E. Hallyn <[email protected]>
Acked-by: Eric Paris <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
include/linux/security.h | 13 +++++++++++++
kernel/module.c | 11 +++++++++++
security/capability.c | 6 ++++++
security/security.c | 5 +++++
4 files changed, 35 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index 3dea6a9..368e539 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -693,6 +693,12 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
* userspace to load a kernel module with the given name.
* @kmod_name name of the module requested by the kernel
* Return 0 if successful.
+ * @kernel_module_from_file:
+ * Load a kernel module from userspace.
+ * @file contains the file structure pointing to the file containing
+ * the kernel module to load. If the module is being loaded from a blob,
+ * this argument will be NULL.
+ * Return 0 if permission is granted.
* @task_fix_setuid:
* Update the module's state after setting one or more of the user
* identity attributes of the current process. The @flags parameter
@@ -1507,6 +1513,7 @@ struct security_operations {
int (*kernel_act_as)(struct cred *new, u32 secid);
int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
int (*kernel_module_request)(char *kmod_name);
+ int (*kernel_module_from_file)(struct file *file);
int (*task_fix_setuid) (struct cred *new, const struct cred *old,
int flags);
int (*task_setpgid) (struct task_struct *p, pid_t pgid);
@@ -1764,6 +1771,7 @@ void security_transfer_creds(struct cred *new, const struct cred *old);
int security_kernel_act_as(struct cred *new, u32 secid);
int security_kernel_create_files_as(struct cred *new, struct inode *inode);
int security_kernel_module_request(char *kmod_name);
+int security_kernel_module_from_file(struct file *file);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags);
int security_task_setpgid(struct task_struct *p, pid_t pgid);
@@ -2277,6 +2285,11 @@ static inline int security_kernel_module_request(char *kmod_name)
return 0;
}

+static inline int security_kernel_module_from_file(struct file *file)
+{
+ return 0;
+}
+
static inline int security_task_fix_setuid(struct cred *new,
const struct cred *old,
int flags)
diff --git a/kernel/module.c b/kernel/module.c
index 321c6b9..261bf82 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -29,6 +29,7 @@
#include <linux/vmalloc.h>
#include <linux/elf.h>
#include <linux/proc_fs.h>
+#include <linux/security.h>
#include <linux/seq_file.h>
#include <linux/syscalls.h>
#include <linux/fcntl.h>
@@ -2489,10 +2490,16 @@ static int elf_header_check(struct load_info *info)
static int copy_module_from_user(const void __user *umod, unsigned long len,
struct load_info *info)
{
+ int err;
+
info->len = len;
if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;

+ err = security_kernel_module_from_file(NULL);
+ if (err)
+ return err;
+
/* Suck in entire file: we'll want most of it. */
info->hdr = vmalloc(info->len);
if (!info->hdr)
@@ -2519,6 +2526,10 @@ static int copy_module_from_fd(int fd, struct load_info *info)
if (!file)
return -ENOEXEC;

+ err = security_kernel_module_from_file(file);
+ if (err)
+ goto out;
+
err = vfs_getattr(file->f_vfsmnt, file->f_dentry, &stat);
if (err)
goto out;
diff --git a/security/capability.c b/security/capability.c
index 61095df..8acb304 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -395,6 +395,11 @@ static int cap_kernel_module_request(char *kmod_name)
return 0;
}

+static int cap_kernel_module_from_file(struct file *file)
+{
+ return 0;
+}
+
static int cap_task_setpgid(struct task_struct *p, pid_t pgid)
{
return 0;
@@ -967,6 +972,7 @@ void __init security_fixup_ops(struct security_operations *ops)
set_to_cap_if_null(ops, kernel_act_as);
set_to_cap_if_null(ops, kernel_create_files_as);
set_to_cap_if_null(ops, kernel_module_request);
+ set_to_cap_if_null(ops, kernel_module_from_file);
set_to_cap_if_null(ops, task_fix_setuid);
set_to_cap_if_null(ops, task_setpgid);
set_to_cap_if_null(ops, task_getpgid);
diff --git a/security/security.c b/security/security.c
index 860aeb3..f7f8695 100644
--- a/security/security.c
+++ b/security/security.c
@@ -799,6 +799,11 @@ int security_kernel_module_request(char *kmod_name)
return security_ops->kernel_module_request(kmod_name);
}

+int security_kernel_module_from_file(struct file *file)
+{
+ return security_ops->kernel_module_from_file(file);
+}
+
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags)
{
--
1.7.9.5

2012-10-05 07:12:32

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 4, 2012 at 8:50 PM, Rusty Russell <[email protected]> wrote:
> Mimi Zohar <[email protected]> writes:
>> Why? Not only have you had these patches sitting for a while, way
>> before you had the kernel module patches, they've been acked/signed off
>> by Kees, Serge, Eric, and myself. All security subtree maintainers.
>> The module patches could have easily been built on top of Kees' small
>> patches. I am really disappointed!
>
> Me too.
>
> Linus' merge window opened on Monday 1-10-2012. One week before that
> was Monday 24-09-2012, which is the nominal close of my merge window.
>
> The patches were sent on 21-09 (Friday for you, the weekend my time).
>
> If I had nothing else to do on Monday, I would have applied it, but we
> spent the week trying to get the module signing patches into shape :(
>
> If I take them now, they need to go through linux-next. That won't
> happen until Monday. I want two days in linux-next, so that people who
> test linux-next get a chance to find issues, so that's Wednesday before
> I send to Linus, which is getting very late into the merge window.
>
> And keep adding two days for every trivial issue which is found :(
>
> It's in my modules-wip branch for 3.8.

Cool; better than not in at all. :) Thanks!

-Kees

--
Kees Cook
Chrome OS Security

2012-10-05 07:35:56

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 4/4] add finit_module syscall to asm-generic

Kees Cook <[email protected]> writes:

> This adds the finit_module syscall to the generic syscall list.

This does ppc. Ben, please Ack.

From: Rusty Russell <[email protected]>
Subject: powerpc: add finit_module syscall.

Signed-off-by: Rusty Russell <[email protected]>

diff --git a/arch/powerpc/include/asm/systbl.h b/arch/powerpc/include/asm/systbl.h
index 8408387..d0b27f8 100644
--- a/arch/powerpc/include/asm/systbl.h
+++ b/arch/powerpc/include/asm/systbl.h
@@ -356,3 +356,4 @@ COMPAT_SYS_SPU(sendmmsg)
SYSCALL_SPU(setns)
COMPAT_SYS(process_vm_readv)
COMPAT_SYS(process_vm_writev)
+SYSCALL(finit_module)
diff --git a/arch/powerpc/include/asm/unistd.h b/arch/powerpc/include/asm/unistd.h
index c683fa3..3d588bb 100644
--- a/arch/powerpc/include/asm/unistd.h
+++ b/arch/powerpc/include/asm/unistd.h
@@ -375,10 +375,11 @@
#define __NR_setns 350
#define __NR_process_vm_readv 351
#define __NR_process_vm_writev 352
+#define __NR_finit_module 353

#ifdef __KERNEL__

-#define __NR_syscalls 353
+#define __NR_syscalls 354

#define __NR__exit __NR_exit
#define NR_syscalls __NR_syscalls

2012-10-05 10:18:39

by James Morris

[permalink] [raw]
Subject: Re: [PATCH 2/4] security: introduce kernel_module_from_file hook

On Thu, 4 Oct 2012, Kees Cook wrote:

> Now that kernel module origins can be reasoned about, provide a hook to
> the LSMs to make policy decisions about the module file. This will let
> Chrome OS enforce that loadable kernel modules can only come from its
> read-only hash-verified root filesystem. Other LSMs can, for example,
> read extended attributes for signatures, etc.
>
> Signed-off-by: Kees Cook <[email protected]>
> Acked-by: Serge E. Hallyn <[email protected]>
> Acked-by: Eric Paris <[email protected]>
> Acked-by: Mimi Zohar <[email protected]>

Acked-by: James Morris <[email protected]>


--
James Morris
<[email protected]>

Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

Kees,

> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)

Given the repeated experience of the last few years--new system calls
that are in essence revisions of older system calls with a 'flags'
argument bolted on to allow more flexible behavior (e.g., accept4(),
dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
on.)--maybe it is worth considering adding a 'flags' bit mask
argument[1] to the finti_module() system call now, to allow for
possible future extensions to the behavior of the interface. What do
you think?

Thanks,

Michael

[1] Yes, I know that init_module() doesn't have a flags argument, but
that interface was added when we'd seen fewer of the kinds of cases
listed above.

2012-10-09 21:58:10

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/10/2012 05:54 AM, Michael Kerrisk wrote:
> Kees,
>
>> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
>
> Given the repeated experience of the last few years--new system calls
> that are in essence revisions of older system calls with a 'flags'
> argument bolted on to allow more flexible behavior (e.g., accept4(),
> dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
> on.)--maybe it is worth considering adding a 'flags' bit mask
> argument[1] to the finti_module() system call now, to allow for
> possible future extensions to the behavior of the interface. What do
> you think?
>
> Thanks,
>
> Michael
>
> [1] Yes, I know that init_module() doesn't have a flags argument, but
> that interface was added when we'd seen fewer of the kinds of cases
> listed above.
>

Then maybe go whole hog and make it an init_module_at() system call
(allowing NULL for the pathname half to implement finit_module())...?

-hpa

Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Tue, Oct 9, 2012 at 11:58 PM, H. Peter Anvin <[email protected]> wrote:
> On 10/10/2012 05:54 AM, Michael Kerrisk wrote:
>> Kees,
>>
>>> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
>>
>> Given the repeated experience of the last few years--new system calls
>> that are in essence revisions of older system calls with a 'flags'
>> argument bolted on to allow more flexible behavior (e.g., accept4(),
>> dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
>> on.)--maybe it is worth considering adding a 'flags' bit mask
>> argument[1] to the finti_module() system call now, to allow for
>> possible future extensions to the behavior of the interface. What do
>> you think?
>>
>> Thanks,
>>
>> Michael
>>
>> [1] Yes, I know that init_module() doesn't have a flags argument, but
>> that interface was added when we'd seen fewer of the kinds of cases
>> listed above.
>>
>
> Then maybe go whole hog and make it an init_module_at() system call
> (allowing NULL for the pathname half to implement finit_module())...?

Good point. A "whole hog" openat()-style interface is worth thinking about too.

--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

2012-10-09 22:09:57

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
> Good point. A "whole hog" openat()-style interface is worth thinking about too.

*Although* you could argue that you can always simply open the module
file first, and that finit_module() is really what we should have had in
the first place. Then you don't need the flags since those would come
from openat().

-hpa

Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

[resending because my mobile device decided it
wanted to send HTML, which of course bounced.]

On Oct 10, 2012 12:09 AM, "H. Peter Anvin" <[email protected]> wrote:
>
> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
> > Good point. A "whole hog" openat()-style interface is worth thinking about too.
>
> *Although* you could argue that you can always simply open the module
> file first, and that finit_module() is really what we should have had in
> the first place. Then you don't need the flags since those would come
> from openat().

But in that case, I'd still stand by my original point: it may be
desirable to have a flags argument to allow future modifications to
the behavior of finit_module() (as opposed to the behavior of the file
open).

2012-10-12 02:30:25

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

"H. Peter Anvin" <[email protected]> writes:

> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
>> Good point. A "whole hog" openat()-style interface is worth thinking about too.
>
> *Although* you could argue that you can always simply open the module
> file first, and that finit_module() is really what we should have had in
> the first place. Then you don't need the flags since those would come
> from openat().

There's no fundamental reason that modules have to be in a file. I'm
thinking of compressed modules, or an initrd which simply includes all
the modules it wants to load in one linear file.

Also, --force options manipulate the module before loading (as did the
now-obsolete module rename option).

Cheers,
Rusty.

Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

Rusty,

On Fri, Oct 12, 2012 at 12:16 AM, Rusty Russell <[email protected]> wrote:
> "H. Peter Anvin" <[email protected]> writes:
>
>> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
>>> Good point. A "whole hog" openat()-style interface is worth thinking about too.
>>
>> *Although* you could argue that you can always simply open the module
>> file first, and that finit_module() is really what we should have had in
>> the first place. Then you don't need the flags since those would come
>> from openat().
>
> There's no fundamental reason that modules have to be in a file. I'm
> thinking of compressed modules, or an initrd which simply includes all
> the modules it wants to load in one linear file.
>
> Also, --force options manipulate the module before loading (as did the
> now-obsolete module rename option).

Sure. But my point that started this subthread was: should we take the
opportunity now to add a 'flags' argument to the new finit_module()
system call, so as to allow flexibility in extending the behavior in
future? There have been so many cases of revised system calls in the
past few years that replaced calls without a 'flags' argument that it
seems worth at least some thought before the API is cast in stone.

Thanks,

Michael

2012-10-18 03:18:37

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

"Michael Kerrisk (man-pages)" <[email protected]> writes:
> Sure. But my point that started this subthread was: should we take the
> opportunity now to add a 'flags' argument to the new finit_module()
> system call, so as to allow flexibility in extending the behavior in
> future? There have been so many cases of revised system calls in the
> past few years that replaced calls without a 'flags' argument that it
> seems worth at least some thought before the API is cast in stone.

(CC's trimmed, Lucas & Jon added; please include them in module
discussions!)

So I tried to think of why we'd want flags; if I could think of a
plausible reason, obviously we should do it now.

I think it would be neat for the force flags (eg. ignoring modversions
or ignoring kernel version). These are the only cases where libkmod
needs to mangle the module.

So here's the patch which adds the flags field, but nothing in there
yet. I'll add the remove flags soon, so libkmod can assume that if the
syscall exists, those flags will work.

Thoughts?
Rusty.

FIX: add flags arg to sys_finit_module()

Thanks to Michael Kerrisk for keeping us honest.

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 32bc035..8cf7b50 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,

asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
unsigned long idx1, unsigned long idx2);
-asmlinkage long sys_finit_module(int fd, const char __user *uargs);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
#endif
diff --git a/kernel/module.c b/kernel/module.c
index 261bf82..8b8d986 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3260,7 +3260,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
return load_module(&info, uargs);
}

-SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
+SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs, int, flags)
{
int err;
struct load_info info = { };
@@ -3269,7 +3269,11 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
if (err)
return err;

- pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);
+ pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
+
+ /* Coming RSN... */
+ if (flags)
+ return -EINVAL;

err = copy_module_from_fd(fd, &info);
if (err)

2012-10-18 04:25:07

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/11/2012 03:16 PM, Rusty Russell wrote:
> "H. Peter Anvin" <[email protected]> writes:
>
>> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
>>> Good point. A "whole hog" openat()-style interface is worth thinking about too.
>>
>> *Although* you could argue that you can always simply open the module
>> file first, and that finit_module() is really what we should have had in
>> the first place. Then you don't need the flags since those would come
>> from openat().
>
> There's no fundamental reason that modules have to be in a file. I'm
> thinking of compressed modules, or an initrd which simply includes all
> the modules it wants to load in one linear file.
>
> Also, --force options manipulate the module before loading (as did the
> now-obsolete module rename option).
>

So perhaps what we *should* have is something that points to the module
to a (buffer, length) in userspace, and the equivalent of the current
init_module() would be open() + mmap() + minit_module() + close()?

-hpa


2012-10-18 05:39:26

by Lucas De Marchi

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 12:12 AM, Rusty Russell <[email protected]> wrote:
> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>> Sure. But my point that started this subthread was: should we take the
>> opportunity now to add a 'flags' argument to the new finit_module()
>> system call, so as to allow flexibility in extending the behavior in
>> future? There have been so many cases of revised system calls in the
>> past few years that replaced calls without a 'flags' argument that it
>> seems worth at least some thought before the API is cast in stone.
>
> (CC's trimmed, Lucas & Jon added; please include them in module
> discussions!)
>
> So I tried to think of why we'd want flags; if I could think of a
> plausible reason, obviously we should do it now.
>
> I think it would be neat for the force flags (eg. ignoring modversions
> or ignoring kernel version). These are the only cases where libkmod
> needs to mangle the module.

Maybe we should put this back into kernel. With an fd we can't mangle
the module anymore to ignore modversions or kernel version.

So yes, I think a 'flags' param is indeed needed.

Side note: force won't work anymore by using init_module() and signed modules.

>
> So here's the patch which adds the flags field, but nothing in there
> yet. I'll add the remove flags soon, so libkmod can assume that if the
> syscall exists, those flags will work.
>
> Thoughts?
> Rusty.
>
> FIX: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 32bc035..8cf7b50 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
>
> asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
> unsigned long idx1, unsigned long idx2);
> -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
> +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
> #endif
> diff --git a/kernel/module.c b/kernel/module.c
> index 261bf82..8b8d986 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3260,7 +3260,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
> return load_module(&info, uargs);
> }
>
> -SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs, int, flags)
> {
> int err;
> struct load_info info = { };
> @@ -3269,7 +3269,11 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
> if (err)
> return err;
>
> - pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);
> + pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
> +
> + /* Coming RSN... */
> + if (flags)
> + return -EINVAL;
>
> err = copy_module_from_fd(fd, &info);
> if (err)


Ack.

Lucas De Marchi

Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 6:24 AM, H. Peter Anvin <[email protected]> wrote:
> On 10/11/2012 03:16 PM, Rusty Russell wrote:
>> "H. Peter Anvin" <[email protected]> writes:
>>
>>> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
>>>> Good point. A "whole hog" openat()-style interface is worth thinking about too.
>>>
>>> *Although* you could argue that you can always simply open the module
>>> file first, and that finit_module() is really what we should have had in
>>> the first place. Then you don't need the flags since those would come
>>> from openat().
>>
>> There's no fundamental reason that modules have to be in a file. I'm
>> thinking of compressed modules, or an initrd which simply includes all
>> the modules it wants to load in one linear file.
>>
>> Also, --force options manipulate the module before loading (as did the
>> now-obsolete module rename option).
>>
>
> So perhaps what we *should* have is something that points to the module
> to a (buffer, length) in userspace, and the equivalent of the current
> init_module() would be open() + mmap() + minit_module() + close()?

So, I don't get it. What are the args you propose for of minit_module()?


--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 5:12 AM, Rusty Russell <[email protected]> wrote:
> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>> Sure. But my point that started this subthread was: should we take the
>> opportunity now to add a 'flags' argument to the new finit_module()
>> system call, so as to allow flexibility in extending the behavior in
>> future? There have been so many cases of revised system calls in the
>> past few years that replaced calls without a 'flags' argument that it
>> seems worth at least some thought before the API is cast in stone.
>
> (CC's trimmed, Lucas & Jon added; please include them in module
> discussions!)
>
> So I tried to think of why we'd want flags; if I could think of a
> plausible reason, obviously we should do it now.
>
> I think it would be neat for the force flags (eg. ignoring modversions
> or ignoring kernel version). These are the only cases where libkmod
> needs to mangle the module.
>
> So here's the patch which adds the flags field, but nothing in there
> yet. I'll add the remove flags soon, so libkmod can assume that if the
> syscall exists, those flags will work.
>
> Thoughts?
> Rusty.
>
> FIX: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.

w00t! Thanks, Rusty ;-).

Acked-by: Michael Kerrisk <[email protected]>

> + if (flags)
> + return -EINVAL;

And thanks for that check. So easy, so obvious, and so often forgotten.

Cheers,

Michael

--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

2012-10-18 14:27:26

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/18/2012 01:05 AM, Michael Kerrisk (man-pages) wrote:
>>
>> So perhaps what we *should* have is something that points to the module
>> to a (buffer, length) in userspace, and the equivalent of the current
>> init_module() would be open() + mmap() + minit_module() + close()?
>
> So, I don't get it. What are the args you propose for of minit_module()?
>

Nevermind, this is what the current init_module() already takes.

So it sounds like Rusty is objecting to the very notion of tying a
module to a file descriptor the way the proposed finit_module() system
call does -- I was confused about the functioning of the *current*
init_module() system call.

Given that, I have to say I now seriously question the value of
finit_module(). The kernel can trivially discover if the pointed-to
memory area is a MAP_SHARED mmap() of a file descriptor and if so which
file descriptor... why can't we handle this behind the scenes?

-hpa

P.S. the man page for init_module(2) is seriously out of date...

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-10-18 15:28:06

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 7:26 AM, H. Peter Anvin <[email protected]> wrote:
> On 10/18/2012 01:05 AM, Michael Kerrisk (man-pages) wrote:
>>>
>>>
>>> So perhaps what we *should* have is something that points to the module
>>> to a (buffer, length) in userspace, and the equivalent of the current
>>> init_module() would be open() + mmap() + minit_module() + close()?
>>
>>
>> So, I don't get it. What are the args you propose for of minit_module()?
>>
>
> Nevermind, this is what the current init_module() already takes.
>
> So it sounds like Rusty is objecting to the very notion of tying a module to
> a file descriptor the way the proposed finit_module() system call does -- I

The goal for finit_module is to make sure we're getting what's on the
filesystem, not an arbitrary blob, so we can reason about it for
security policy.

> was confused about the functioning of the *current* init_module() system
> call.
>
> Given that, I have to say I now seriously question the value of
> finit_module(). The kernel can trivially discover if the pointed-to memory
> area is a MAP_SHARED mmap() of a file descriptor and if so which file
> descriptor... why can't we handle this behind the scenes?

This makes me very nervous. I worry that it adds needless complexity
(it'd be many more checks besides "is it MAP_SHARED?", like "does the
memory region show the whole file?" "is the offset zero?" etc). Also
are we sure the memory area would be truly be unmodifiable in the case
where the filesystem is read-only?

-Kees

--
Kees Cook
Chrome OS Security

2012-10-18 15:37:19

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/18/2012 08:28 AM, Kees Cook wrote:
>
> The goal for finit_module is to make sure we're getting what's on the
> filesystem, not an arbitrary blob, so we can reason about it for
> security policy.
>

Yes, I get that... although I'm starting to think that that might
actually be a really bad idea.

>> was confused about the functioning of the *current* init_module() system
>> call.
>>
>> Given that, I have to say I now seriously question the value of
>> finit_module(). The kernel can trivially discover if the pointed-to memory
>> area is a MAP_SHARED mmap() of a file descriptor and if so which file
>> descriptor... why can't we handle this behind the scenes?
>
> This makes me very nervous. I worry that it adds needless complexity
> (it'd be many more checks besides "is it MAP_SHARED?", like "does the
> memory region show the whole file?" "is the offset zero?" etc). Also
> are we sure the memory area would be truly be unmodifiable in the case
> where the filesystem is read-only?

You may need to check for PROT_READONLY as well.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-10-19 02:28:25

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

"H. Peter Anvin" <[email protected]> writes:
> Given that, I have to say I now seriously question the value of
> finit_module(). The kernel can trivially discover if the pointed-to
> memory area is a MAP_SHARED mmap() of a file descriptor and if so which
> file descriptor... why can't we handle this behind the scenes?

It is a bit more indirect, but also in practice it's a bit trickier than
that. We need to ensure the memory doesn't change underneath us and
stays attached to that fd. I can easily see that code slipping and
ending in an exploit.

But that may be my irrational fear of the mm :)

Cheers,
Rusty.

2012-10-19 02:55:29

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/18/2012 07:23 PM, Rusty Russell wrote:
> "H. Peter Anvin" <[email protected]> writes:
>> Given that, I have to say I now seriously question the value of
>> finit_module(). The kernel can trivially discover if the pointed-to
>> memory area is a MAP_SHARED mmap() of a file descriptor and if so which
>> file descriptor... why can't we handle this behind the scenes?
>
> It is a bit more indirect, but also in practice it's a bit trickier than
> that. We need to ensure the memory doesn't change underneath us and
> stays attached to that fd. I can easily see that code slipping and
> ending in an exploit.
>
> But that may be my irrational fear of the mm :)

You have to do the same thing with a file/file descriptor, I would think.

However, I keep wondering about the use case for this, as opposed to
signatures.

-hpa


--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-10-19 11:40:16

by Alon Ziv

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

H. Peter Anvin <hpa <at> zytor.com> writes:
> > It is a bit more indirect, but also in practice it's a bit trickier than
> > that. We need to ensure the memory doesn't change underneath us and
> > stays attached to that fd. I can easily see that code slipping and
> > ending in an exploit.
> >
> > But that may be my irrational fear of the mm :)
>
> You have to do the same thing with a file/file descriptor, I would think.
>
> However, I keep wondering about the use case for this, as opposed to
> signatures.

Two things:
1. finit_module() lets LSMs make decisions based on full information on the
module to be loaded
2. On some systems (such as Chromium OS) we have a trusted root OS (e.g. the
entire root filesystem is protected using dm-verity); requiring signatures
on top of this is a waste of resources

2012-10-20 04:14:54

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

"H. Peter Anvin" <[email protected]> writes:
> On 10/18/2012 07:23 PM, Rusty Russell wrote:
>> "H. Peter Anvin" <[email protected]> writes:
>>> Given that, I have to say I now seriously question the value of
>>> finit_module(). The kernel can trivially discover if the pointed-to
>>> memory area is a MAP_SHARED mmap() of a file descriptor and if so which
>>> file descriptor... why can't we handle this behind the scenes?
>>
>> It is a bit more indirect, but also in practice it's a bit trickier than
>> that. We need to ensure the memory doesn't change underneath us and
>> stays attached to that fd. I can easily see that code slipping and
>> ending in an exploit.
>>
>> But that may be my irrational fear of the mm :)
>
> You have to do the same thing with a file/file descriptor, I would think.

After the fget(fd), it can't change where it's attached to though.
Ensuring that the fd behind the shared region is trusted and doesn't
change between the check and the read has more atomicity issues.

> However, I keep wondering about the use case for this, as opposed to
> signatures.

The IMA people are signing every file in xattrs; this makes it trivial
for them to extend the same mechanism to kernel modules (though they'll
probably want to add xattrs to our cpio support, but bsdcpio et al
already have cpio-with-xattrs so I doubt it'll be hard).

And Kees simply has a known-secure partition, IIUC, which makes their
verification step pretty trivial.

The opportunity to add a flags arg is just the icing on the cake, but I
think the combination is compelling.

Cheers,
Rusty.

2012-10-23 02:02:27

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

"Michael Kerrisk (man-pages)" <[email protected]> writes:
>> FIX: add flags arg to sys_finit_module()
>>
>> Thanks to Michael Kerrisk for keeping us honest.
>
> w00t! Thanks, Rusty ;-).
>
> Acked-by: Michael Kerrisk <[email protected]>

Here's the version I ended up with when I added two flags.

Lucas, is this useful to you?

BTW Michael: why aren't the syscall man pages in the kernel source?

Thanks,
Rusty.

module: add flags arg to sys_finit_module()

Thanks to Michael Kerrisk for keeping us honest. These flags are actually
useful for eliminating the only case where kmod has to mangle a module's
internals: for overriding module versioning.

Signed-off-by: Rusty Russell <[email protected]>

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 32bc035..8cf7b50 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,

asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
unsigned long idx1, unsigned long idx2);
-asmlinkage long sys_finit_module(int fd, const char __user *uargs);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
#endif
diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
new file mode 100644
index 0000000..38da425
--- /dev/null
+++ b/include/uapi/linux/module.h
@@ -0,0 +1,8 @@
+#ifndef _UAPI_LINUX_MODULE_H
+#define _UAPI_LINUX_MODULE_H
+
+/* Flags for sys_finit_module: */
+#define MODULE_INIT_IGNORE_MODVERSIONS 1
+#define MODULE_INIT_IGNORE_VERMAGIC 2
+
+#endif /* _UAPI_LINUX_MODULE_H */
diff --git a/kernel/module.c b/kernel/module.c
index 261bf82..55b49cd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -61,6 +61,7 @@
#include <linux/pfn.h>
#include <linux/bsearch.h>
#include <linux/fips.h>
+#include <uapi/linux/module.h>
#include "module-internal.h"

#define CREATE_TRACE_POINTS
@@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
vfree(info->hdr);
}

-static int rewrite_section_headers(struct load_info *info)
+static int rewrite_section_headers(struct load_info *info, int flags)
{
unsigned int i;

@@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info *info)
}

/* Track but don't keep modinfo and version sections. */
- info->index.vers = find_sec(info, "__versions");
+ if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
+ info->index.vers = 0; /* Pretend no __versions section! */
+ else
+ info->index.vers = find_sec(info, "__versions");
info->index.info = find_sec(info, ".modinfo");
info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
@@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info *info)
* Return the temporary module pointer (we'll replace it with the final
* one when we move the module sections around).
*/
-static struct module *setup_load_info(struct load_info *info)
+static struct module *setup_load_info(struct load_info *info, int flags)
{
unsigned int i;
int err;
@@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info *info)
info->secstrings = (void *)info->hdr
+ info->sechdrs[info->hdr->e_shstrndx].sh_offset;

- err = rewrite_section_headers(info);
+ err = rewrite_section_headers(info, flags);
if (err)
return ERR_PTR(err);

@@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct load_info *info)
return mod;
}

-static int check_modinfo(struct module *mod, struct load_info *info)
+static int check_modinfo(struct module *mod, struct load_info *info, int flags)
{
const char *modmagic = get_modinfo(info, "vermagic");
int err;

+ if (flags & MODULE_INIT_IGNORE_VERMAGIC)
+ modmagic = NULL;
+
/* This is allowed: modprobe --force will invalidate it. */
if (!modmagic) {
err = try_to_force_load(mod, "bad vermagic");
@@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
return 0;
}

-static struct module *layout_and_allocate(struct load_info *info)
+static struct module *layout_and_allocate(struct load_info *info, int flags)
{
/* Module within temporary copy. */
struct module *mod;
Elf_Shdr *pcpusec;
int err;

- mod = setup_load_info(info);
+ mod = setup_load_info(info, flags);
if (IS_ERR(mod))
return mod;

- err = check_modinfo(mod, info);
+ err = check_modinfo(mod, info, flags);
if (err)
return ERR_PTR(err);

@@ -3094,7 +3102,8 @@ static int may_init_module(void)

/* 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)
+static int load_module(struct load_info *info, const char __user *uargs,
+ int flags)
{
struct module *mod, *old;
long err;
@@ -3108,7 +3117,7 @@ static int load_module(struct load_info *info, const char __user *uargs)
goto free_copy;

/* Figure out module layout, and allocate all the memory. */
- mod = layout_and_allocate(info);
+ mod = layout_and_allocate(info, flags);
if (IS_ERR(mod)) {
err = PTR_ERR(mod);
goto free_copy;
@@ -3257,10 +3269,10 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
if (err)
return err;

- return load_module(&info, uargs);
+ return load_module(&info, uargs, 0);
}

-SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
+SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
{
int err;
struct load_info info = { };
@@ -3269,13 +3281,17 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
if (err)
return err;

- pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);
+ pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
+
+ if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS
+ |MODULE_INIT_IGNORE_VERMAGIC))
+ return -EINVAL;

err = copy_module_from_fd(fd, &info);
if (err)
return err;

- return load_module(&info, uargs);
+ return load_module(&info, uargs, flags);
}

static inline int within(unsigned long addr, void *start, unsigned long size)

2012-10-23 02:37:59

by Lucas De Marchi

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <[email protected]> wrote:
> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <[email protected]>
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?
>
> Thanks,
> Rusty.
>
> module: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest. These flags are actually
> useful for eliminating the only case where kmod has to mangle a module's
> internals: for overriding module versioning.
>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 32bc035..8cf7b50 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
>
> asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
> unsigned long idx1, unsigned long idx2);
> -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
> +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
> #endif
> diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
> new file mode 100644
> index 0000000..38da425
> --- /dev/null
> +++ b/include/uapi/linux/module.h
> @@ -0,0 +1,8 @@
> +#ifndef _UAPI_LINUX_MODULE_H
> +#define _UAPI_LINUX_MODULE_H
> +
> +/* Flags for sys_finit_module: */
> +#define MODULE_INIT_IGNORE_MODVERSIONS 1
> +#define MODULE_INIT_IGNORE_VERMAGIC 2
> +
> +#endif /* _UAPI_LINUX_MODULE_H */
> diff --git a/kernel/module.c b/kernel/module.c
> index 261bf82..55b49cd 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -61,6 +61,7 @@
> #include <linux/pfn.h>
> #include <linux/bsearch.h>
> #include <linux/fips.h>
> +#include <uapi/linux/module.h>
> #include "module-internal.h"
>
> #define CREATE_TRACE_POINTS
> @@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
> vfree(info->hdr);
> }
>
> -static int rewrite_section_headers(struct load_info *info)
> +static int rewrite_section_headers(struct load_info *info, int flags)
> {
> unsigned int i;
>
> @@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info *info)
> }
>
> /* Track but don't keep modinfo and version sections. */
> - info->index.vers = find_sec(info, "__versions");
> + if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
> + info->index.vers = 0; /* Pretend no __versions section! */
> + else
> + info->index.vers = find_sec(info, "__versions");
> info->index.info = find_sec(info, ".modinfo");
> info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
> info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
> @@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info *info)
> * Return the temporary module pointer (we'll replace it with the final
> * one when we move the module sections around).
> */
> -static struct module *setup_load_info(struct load_info *info)
> +static struct module *setup_load_info(struct load_info *info, int flags)
> {
> unsigned int i;
> int err;
> @@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info *info)
> info->secstrings = (void *)info->hdr
> + info->sechdrs[info->hdr->e_shstrndx].sh_offset;
>
> - err = rewrite_section_headers(info);
> + err = rewrite_section_headers(info, flags);
> if (err)
> return ERR_PTR(err);
>
> @@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct load_info *info)
> return mod;
> }
>
> -static int check_modinfo(struct module *mod, struct load_info *info)
> +static int check_modinfo(struct module *mod, struct load_info *info, int flags)
> {
> const char *modmagic = get_modinfo(info, "vermagic");
> int err;
>
> + if (flags & MODULE_INIT_IGNORE_VERMAGIC)
> + modmagic = NULL;
> +
> /* This is allowed: modprobe --force will invalidate it. */
> if (!modmagic) {
> err = try_to_force_load(mod, "bad vermagic");
> @@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
> return 0;
> }
>
> -static struct module *layout_and_allocate(struct load_info *info)
> +static struct module *layout_and_allocate(struct load_info *info, int flags)
> {
> /* Module within temporary copy. */
> struct module *mod;
> Elf_Shdr *pcpusec;
> int err;
>
> - mod = setup_load_info(info);
> + mod = setup_load_info(info, flags);
> if (IS_ERR(mod))
> return mod;
>
> - err = check_modinfo(mod, info);
> + err = check_modinfo(mod, info, flags);
> if (err)
> return ERR_PTR(err);
>
> @@ -3094,7 +3102,8 @@ static int may_init_module(void)
>
> /* 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)
> +static int load_module(struct load_info *info, const char __user *uargs,
> + int flags)
> {
> struct module *mod, *old;
> long err;
> @@ -3108,7 +3117,7 @@ static int load_module(struct load_info *info, const char __user *uargs)
> goto free_copy;
>
> /* Figure out module layout, and allocate all the memory. */
> - mod = layout_and_allocate(info);
> + mod = layout_and_allocate(info, flags);
> if (IS_ERR(mod)) {
> err = PTR_ERR(mod);
> goto free_copy;
> @@ -3257,10 +3269,10 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
> if (err)
> return err;
>
> - return load_module(&info, uargs);
> + return load_module(&info, uargs, 0);

I wonder if we shouldn't get a new init_module2() as well, adding the
flags parameter. Of course this would be in another patch.

My worries are that for compressed modules we still need to use
init_module() and then --force won't work with signed modules.


> }
>
> -SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
> +SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
> {
> int err;
> struct load_info info = { };
> @@ -3269,13 +3281,17 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
> if (err)
> return err;
>
> - pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);
> + pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
> +
> + if (flags & ~(MODULE_INIT_IGNORE_MODVERSIONS
> + |MODULE_INIT_IGNORE_VERMAGIC))
> + return -EINVAL;
>
> err = copy_module_from_fd(fd, &info);
> if (err)
> return err;
>
> - return load_module(&info, uargs);
> + return load_module(&info, uargs, flags);
> }
>
> static inline int within(unsigned long addr, void *start, unsigned long size)

Acked-by: Lucas De Marchi <[email protected]>

2012-10-23 03:40:50

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
<[email protected]> wrote:
> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <[email protected]> wrote:
>> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>>>> FIX: add flags arg to sys_finit_module()
>>>>
>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>
>>> w00t! Thanks, Rusty ;-).
>>>
>>> Acked-by: Michael Kerrisk <[email protected]>
>>
>> Here's the version I ended up with when I added two flags.
>>
>> Lucas, is this useful to you?
>>
>> BTW Michael: why aren't the syscall man pages in the kernel source?
>>
>> Thanks,
>> Rusty.
>>
>> module: add flags arg to sys_finit_module()
>>
>> Thanks to Michael Kerrisk for keeping us honest. These flags are actually
>> useful for eliminating the only case where kmod has to mangle a module's
>> internals: for overriding module versioning.
>>
>> Signed-off-by: Rusty Russell <[email protected]>

Acked-by: Kees Cook <[email protected]>

> I wonder if we shouldn't get a new init_module2() as well, adding the
> flags parameter. Of course this would be in another patch.
>
> My worries are that for compressed modules we still need to use
> init_module() and then --force won't work with signed modules.

For those cases, I think it should remain up to userspace to do the
decompress and use init_module(). The code I'd written for patching
module-init-tools basically just kept the fd around if it didn't need
to mangle the module, and it would use finit_module (written before
the flags argument was added):

/* request kernel linkage */
- ret = init_module(module->data, module->len, opts);
+ if (fd < 0)
+ ret = init_module(module->data, module->len, opts);
+ else {
+ ret = finit_module(fd, opts);
+ if (ret != 0 && errno == ENOSYS)
+ ret = init_module(module->data, module->len, opts);
+ }
if (ret != 0) {

(And yes, I realize kmod is what'll actually be getting this logic.
This was for my testing in Chrome OS, which is still using
module-init-tools.)

-Kees

--
Kees Cook
Chrome OS Security

2012-10-23 04:08:54

by Lucas De Marchi

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook <[email protected]> wrote:
> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
> <[email protected]> wrote:
>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <[email protected]> wrote:
>>> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>>>>> FIX: add flags arg to sys_finit_module()
>>>>>
>>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>>
>>>> w00t! Thanks, Rusty ;-).
>>>>
>>>> Acked-by: Michael Kerrisk <[email protected]>
>>>
>>> Here's the version I ended up with when I added two flags.
>>>
>>> Lucas, is this useful to you?
>>>
>>> BTW Michael: why aren't the syscall man pages in the kernel source?
>>>
>>> Thanks,
>>> Rusty.
>>>
>>> module: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest. These flags are actually
>>> useful for eliminating the only case where kmod has to mangle a module's
>>> internals: for overriding module versioning.
>>>
>>> Signed-off-by: Rusty Russell <[email protected]>
>
> Acked-by: Kees Cook <[email protected]>
>
>> I wonder if we shouldn't get a new init_module2() as well, adding the
>> flags parameter. Of course this would be in another patch.
>>
>> My worries are that for compressed modules we still need to use
>> init_module() and then --force won't work with signed modules.
>
> For those cases, I think it should remain up to userspace to do the
> decompress and use init_module(). The code I'd written for patching
> module-init-tools basically just kept the fd around if it didn't need
> to mangle the module, and it would use finit_module (written before
> the flags argument was added):
>
> /* request kernel linkage */
> - ret = init_module(module->data, module->len, opts);
> + if (fd < 0)
> + ret = init_module(module->data, module->len, opts);
> + else {
> + ret = finit_module(fd, opts);
> + if (ret != 0 && errno == ENOSYS)
> + ret = init_module(module->data, module->len, opts);
> + }
> if (ret != 0) {
>
> (And yes, I realize kmod is what'll actually be getting this logic.
> This was for my testing in Chrome OS, which is still using
> module-init-tools.)

sure... but do you realize this will fail in case kernel is checking
module signature and we passed --force to modprobe (therefore mangled
the decompressed memory area)?


Lucas De Marchi

Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell <[email protected]> wrote:
> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <[email protected]>
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?

So, this more or less reached the status of an FAQ, and here are my thoughts:
http://www.kernel.org/doc/man-pages/todo.html#migrate_to_kernel_source

Cheers,

Michael


--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/

2012-10-23 15:42:55

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
<[email protected]> wrote:
> On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook <[email protected]> wrote:
>> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>> <[email protected]> wrote:
>>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <[email protected]> wrote:
>>>> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>>>>>> FIX: add flags arg to sys_finit_module()
>>>>>>
>>>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>>>
>>>>> w00t! Thanks, Rusty ;-).
>>>>>
>>>>> Acked-by: Michael Kerrisk <[email protected]>
>>>>
>>>> Here's the version I ended up with when I added two flags.
>>>>
>>>> Lucas, is this useful to you?
>>>>
>>>> BTW Michael: why aren't the syscall man pages in the kernel source?
>>>>
>>>> Thanks,
>>>> Rusty.
>>>>
>>>> module: add flags arg to sys_finit_module()
>>>>
>>>> Thanks to Michael Kerrisk for keeping us honest. These flags are actually
>>>> useful for eliminating the only case where kmod has to mangle a module's
>>>> internals: for overriding module versioning.
>>>>
>>>> Signed-off-by: Rusty Russell <[email protected]>
>>
>> Acked-by: Kees Cook <[email protected]>
>>
>>> I wonder if we shouldn't get a new init_module2() as well, adding the
>>> flags parameter. Of course this would be in another patch.
>>>
>>> My worries are that for compressed modules we still need to use
>>> init_module() and then --force won't work with signed modules.
>>
>> For those cases, I think it should remain up to userspace to do the
>> decompress and use init_module(). The code I'd written for patching
>> module-init-tools basically just kept the fd around if it didn't need
>> to mangle the module, and it would use finit_module (written before
>> the flags argument was added):
>>
>> /* request kernel linkage */
>> - ret = init_module(module->data, module->len, opts);
>> + if (fd < 0)
>> + ret = init_module(module->data, module->len, opts);
>> + else {
>> + ret = finit_module(fd, opts);
>> + if (ret != 0 && errno == ENOSYS)
>> + ret = init_module(module->data, module->len, opts);
>> + }
>> if (ret != 0) {
>>
>> (And yes, I realize kmod is what'll actually be getting this logic.
>> This was for my testing in Chrome OS, which is still using
>> module-init-tools.)
>
> sure... but do you realize this will fail in case kernel is checking
> module signature and we passed --force to modprobe (therefore mangled
> the decompressed memory area)?

Hm, yeah, userspace mangling of a module plus signing would fail.
Seems like mangling and signing aren't compatible. Doing it in
kernel-space (as now written for finit_module) solves that, but it
means that now compression isn't possible if you need both signing and
mangling.

I'm not a user of signing, compression, or mangling, so I'm probably a
bit unimaginative here. It seems like the case for needing all three
is pretty uncommon. (e.g. if you're doing compression, you're probably
building embedded images, which means you're unlikely to need
--force.)

-Kees

--
Kees Cook
Chrome OS Security

2012-10-23 15:45:34

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/23/2012 08:42 AM, Kees Cook wrote:
>
> Hm, yeah, userspace mangling of a module plus signing would fail.
> Seems like mangling and signing aren't compatible. Doing it in
> kernel-space (as now written for finit_module) solves that, but it
> means that now compression isn't possible if you need both signing and
> mangling.
>
> I'm not a user of signing, compression, or mangling, so I'm probably a
> bit unimaginative here. It seems like the case for needing all three
> is pretty uncommon. (e.g. if you're doing compression, you're probably
> building embedded images, which means you're unlikely to need
> --force.)
>

In particular, mangling and signing aren't compatible... however,
signing and compression should be just fine (sign before compression).

-hpa


--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2012-10-23 16:25:39

by Lucas De Marchi

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook <[email protected]> wrote:
> On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
> <[email protected]> wrote:
>> On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook <[email protected]> wrote:
>>> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>>> <[email protected]> wrote:
>>>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <[email protected]> wrote:
>>>>> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>>>>>>> FIX: add flags arg to sys_finit_module()
>>>>>>>
>>>>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>>>>
>>>>>> w00t! Thanks, Rusty ;-).
>>>>>>
>>>>>> Acked-by: Michael Kerrisk <[email protected]>
>>>>>
>>>>> Here's the version I ended up with when I added two flags.
>>>>>
>>>>> Lucas, is this useful to you?
>>>>>
>>>>> BTW Michael: why aren't the syscall man pages in the kernel source?
>>>>>
>>>>> Thanks,
>>>>> Rusty.
>>>>>
>>>>> module: add flags arg to sys_finit_module()
>>>>>
>>>>> Thanks to Michael Kerrisk for keeping us honest. These flags are actually
>>>>> useful for eliminating the only case where kmod has to mangle a module's
>>>>> internals: for overriding module versioning.
>>>>>
>>>>> Signed-off-by: Rusty Russell <[email protected]>
>>>
>>> Acked-by: Kees Cook <[email protected]>
>>>
>>>> I wonder if we shouldn't get a new init_module2() as well, adding the
>>>> flags parameter. Of course this would be in another patch.
>>>>
>>>> My worries are that for compressed modules we still need to use
>>>> init_module() and then --force won't work with signed modules.
>>>
>>> For those cases, I think it should remain up to userspace to do the
>>> decompress and use init_module(). The code I'd written for patching
>>> module-init-tools basically just kept the fd around if it didn't need
>>> to mangle the module, and it would use finit_module (written before
>>> the flags argument was added):
>>>
>>> /* request kernel linkage */
>>> - ret = init_module(module->data, module->len, opts);
>>> + if (fd < 0)
>>> + ret = init_module(module->data, module->len, opts);
>>> + else {
>>> + ret = finit_module(fd, opts);
>>> + if (ret != 0 && errno == ENOSYS)
>>> + ret = init_module(module->data, module->len, opts);
>>> + }
>>> if (ret != 0) {
>>>
>>> (And yes, I realize kmod is what'll actually be getting this logic.
>>> This was for my testing in Chrome OS, which is still using
>>> module-init-tools.)
>>
>> sure... but do you realize this will fail in case kernel is checking
>> module signature and we passed --force to modprobe (therefore mangled
>> the decompressed memory area)?
>
> Hm, yeah, userspace mangling of a module plus signing would fail.
> Seems like mangling and signing aren't compatible. Doing it in
> kernel-space (as now written for finit_module) solves that, but it
> means that now compression isn't possible if you need both signing and
> mangling.
>
> I'm not a user of signing, compression, or mangling, so I'm probably a
> bit unimaginative here. It seems like the case for needing all three
> is pretty uncommon. (e.g. if you're doing compression, you're probably
> building embedded images, which means you're unlikely to need
> --force.)


Some desktop distros ship compressed modules by default. I received
feedback from distros some months ago that this is basically because
of the disk space, not performance. However some measurements I did
in a regular laptop with spinning disk showed a small advantage
performance-wise, too (with SSD is another story and uncompressed
wins by a large margin)

Since this only affects users of --force option, I think it only
affects module developers, who could uncompress the module, call
depmod again, and modprobe it. ( Mixing compressed and uncompressed
modules used not to work in module-init-tools and earlier versions of
kmod wrt dependencies, but now it should be a seamless operation )


Lucas De Marchi

2012-10-24 11:29:57

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

Lucas De Marchi <[email protected]> writes:
> On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook <[email protected]> wrote:
>> On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
>> <[email protected]> wrote:
>>> sure... but do you realize this will fail in case kernel is checking
>>> module signature and we passed --force to modprobe (therefore mangled
>>> the decompressed memory area)?
>>
>> Hm, yeah, userspace mangling of a module plus signing would fail.
>> Seems like mangling and signing aren't compatible. Doing it in
>> kernel-space (as now written for finit_module) solves that, but it
>> means that now compression isn't possible if you need both signing and
>> mangling.

Signing and mangling are always incompatible, of course.

Compressed modules breaks Kees' (and IMA's) requirement to have an fd
attached, unless the kernel does the decompression. We have that code
already, in fact.

It would be easy to add a config option the recognize the compression
magic and uncompress in-kernel. That would happen after the signature
check, of course, and Just Work.

Cheers,
Rusty.

2012-10-30 21:58:00

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

On Mon, Oct 22, 2012 at 12:39 AM, Rusty Russell <[email protected]> wrote:
> "Michael Kerrisk (man-pages)" <[email protected]> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <[email protected]>
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?
>
> Thanks,
> Rusty.
>
> module: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest. These flags are actually
> useful for eliminating the only case where kmod has to mangle a module's
> internals: for overriding module versioning.
>
> Signed-off-by: Rusty Russell <[email protected]>

Rusty,

I haven't seen this land in your modules-next tree. I just wanted to
make sure it hadn't gotten lost. I'd like to do some kmod tests
against linux-next, but I've been waiting for this to appear.

I acked this before, but as long as I'm replying again:

Acked-by: Kees Cook <[email protected]>

-Kees

--
Kees Cook
Chrome OS Security

2012-11-01 01:03:22

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH 1/4] module: add syscall to load module from fd

Kees Cook <[email protected]> writes:
> Rusty,
>
> I haven't seen this land in your modules-next tree. I just wanted to
> make sure it hadn't gotten lost. I'd like to do some kmod tests
> against linux-next, but I've been waiting for this to appear.

Yes, sorting that out now, they should be in tomorrow's linux-next.
And I've sent the ppc patch to linuxppc-dev for Acks.

Thanks for the prod,
Rusty.