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

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

[Kees Cook]

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 <keescook@chromium.org>
---
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                  |  343 +++++++++++++++++++++++--------------
 kernel/sys_ni.c                  |    1 +
 5 files changed, 217 insertions(+), 130 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 4edbd9c..afe2f69 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>
@@ -2399,48 +2400,105 @@ static inline void kmemleak_load_module(const struct module *mod,
 }
 #endif
 
+/* Sanity checks against invalid binaries, wrong arch, weird elf version. */
+static int check_info(struct load_info *info)
+{
+	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;
+
+	return 0;
+}
+
 /* Sets info->hdr and info->len. */
-static int copy_and_check(struct load_info *info,
-			  const void __user *umod, unsigned long len,
-			  const char __user *uargs)
+static int copy_module_from_user(const void __user *umod, unsigned long len,
+				  struct load_info *info)
 {
 	int err;
-	Elf_Ehdr *hdr;
 
-	if (len < sizeof(*hdr))
+	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) {
+	if (copy_from_user(info->hdr, umod, info->len) != 0) {
 		err = -EFAULT;
 		goto free_hdr;
 	}
 
-	/* 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;
+	err = check_info(info);
+	if (err)
 		goto free_hdr;
+
+	return err;
+
+free_hdr:
+	vfree(info->hdr);
+	return err;
+}
+
+/* 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 out;
+
+	if (stat.size > INT_MAX) {
+		err = -EFBIG;
+		goto out;
+	}
+	info->hdr = vmalloc(stat.size);
+	if (!info->hdr) {
+		err = -ENOMEM;
+		goto out;
 	}
 
-	if (hdr->e_shoff >= len ||
-	    hdr->e_shnum * sizeof(Elf_Shdr) > len - hdr->e_shoff) {
-		err = -ENOEXEC;
-		goto free_hdr;
+	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;
 
-	info->hdr = hdr;
-	info->len = len;
-	return 0;
+	err = check_info(info);
+	if (err)
+		vfree(info->hdr);
 
-free_hdr:
-	vfree(hdr);
+out:
+	fput(file);
 	return err;
 }
 
@@ -2861,26 +2919,100 @@ static int post_relocation(struct module *mod, const struct load_info *info)
 	return module_finalize(info->hdr, info->sechdrs, mod);
 }
 
+/* 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(&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!  Wake up anyone waiting for it. */
+	mod->state = MODULE_STATE_LIVE;
+	wake_up(&module_wq);
+	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);
+
+	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;
 	long err;
 
-	pr_debug("load_module: umod=%p, len=%lu, uargs=%p\n",
-	       umod, len, uargs);
-
-	/* Copy in the blobs from userspace, check they are vaguely sane. */
-	err = copy_and_check(&info, umod, len, uargs);
-	if (err)
-		return ERR_PTR(err);
-
 	/* 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;
@@ -2893,25 +3025,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;
 
@@ -2941,14 +3073,14 @@ static struct module *load_module(void __user *umod,
 	}
 
 	/* 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);
 
@@ -2959,16 +3091,17 @@ static struct module *load_module(void __user *umod,
 		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);
@@ -2977,7 +3110,7 @@ static struct module *load_module(void __user *umod,
 	module_bug_cleanup(mod);
 
  ddebug:
-	dynamic_debug_remove(info.debug);
+	dynamic_debug_remove(info->debug);
  unlock:
 	mutex_unlock(&module_mutex);
 	synchronize_sched();
@@ -2989,106 +3122,56 @@ static struct module *load_module(void __user *umod,
  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);
+	free_copy(info);
+	return err;
 }
 
-/* Call module constructors. */
-static void do_mod_ctors(struct module *mod)
+static int may_init_module(void)
 {
-#ifdef CONFIG_CONSTRUCTORS
-	unsigned long i;
+	if (!capable(CAP_SYS_MODULE) || modules_disabled)
+		return -EPERM;
 
-	for (i = 0; i < mod->num_ctors; i++)
-		mod->ctors[i]();
-#endif
+	return 0;
 }
 
-/* This is where the real work happens */
-SYSCALL_DEFINE3(init_module, void __user *, umod,
-		unsigned long, len, const char __user *, uargs)
+SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
 {
-	struct module *mod;
-	int ret = 0;
+	int err;
+	struct load_info info = { };
 
-	/* Must have permission */
-	if (!capable(CAP_SYS_MODULE) || modules_disabled)
-		return -EPERM;
+	err = may_init_module();
+	if (err)
+		return err;
 
-	/* Do all the hard work */
-	mod = load_module(umod, len, uargs);
-	if (IS_ERR(mod))
-		return PTR_ERR(mod);
+	pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);
 
-	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);
+	err = copy_module_from_fd(fd, &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(&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_DEFINE3(init_module, void __user *, umod,
+		unsigned long, len, const char __user *, uargs)
+{
+	int err;
+	struct load_info info = { };
 
-	/* Now it's a first class citizen!  Wake up anyone waiting for it. */
-	mod->state = MODULE_STATE_LIVE;
-	wake_up(&module_wq);
-	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("init_module: umod=%p, len=%lu, uargs=%p\n",
+	       umod, len, 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);
+	err = copy_module_from_user(umod, len, &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.0.4

[PATCH 2/4] security: introduce kernel_module_from_file hook

[Kees Cook]

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 <keescook@chromium.org>
Acked-by: Serge E. Hallyn <serge.hallyn@canonical.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 include/linux/security.h |   13 +++++++++++++
 kernel/module.c          |    9 +++++++++
 security/capability.c    |    6 ++++++
 security/security.c      |    5 +++++
 4 files changed, 33 insertions(+), 0 deletions(-)

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 afe2f69..511b8e9 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>
@@ -2430,6 +2431,10 @@ static int copy_module_from_user(const void __user *umod, unsigned long 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)
@@ -2464,6 +2469,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.0.4

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

[Kees Cook]

Add finit_module syscall to the ARM syscall list.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/include/asm/unistd.h |    1 +
 arch/arm/kernel/calls.S       |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index 0cab47d..904b579 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -404,6 +404,7 @@
 #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)
+#define __NR_finit_module		(__NR_SYSCALL_BASE+378)
 
 /*
  * The following SWIs are ARM private.
diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S
index 463ff4a..54161ac 100644
--- a/arch/arm/kernel/calls.S
+++ b/arch/arm/kernel/calls.S
@@ -387,6 +387,7 @@
 /* 375 */	CALL(sys_setns)
 		CALL(sys_process_vm_readv)
 		CALL(sys_process_vm_writev)
+		CALL(sys_finit_module)
 #ifndef syscalls_counted
 .equ syscalls_padding, ((NR_syscalls + 3) & ~3) - NR_syscalls
 #define syscalls_counted
-- 
1.7.0.4

[PATCH 4/4] add finit_module syscall to asm-generic

[Kees Cook]

This adds the finit_module syscall to the generic syscall list.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/unistd.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/include/asm-generic/unistd.h b/include/asm-generic/unistd.h
index 991ef01..671d746 100644
--- a/include/asm-generic/unistd.h
+++ b/include/asm-generic/unistd.h
@@ -691,9 +691,11 @@ __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_finit_module 272
+__SYSCALL(__NR_finit_module, sys_finit_module)
 
 #undef __NR_syscalls
-#define __NR_syscalls 272
+#define __NR_syscalls 273
 
 /*
  * All syscalls below here should go away really,
-- 
1.7.0.4

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

[James Morris]

On Thu, 20 Sep 2012, Kees Cook wrote:

> 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.

Just out of interest, has anyone else expressed interest in using this 
feature?



-- 
James Morris
<jmorris@namei.org>

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

[Kees Cook]

On Thu, Sep 20, 2012 at 7:22 PM, James Morris <jmorris@namei.org> wrote:
> On Thu, 20 Sep 2012, Kees Cook wrote:
>
>> 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.
>
> Just out of interest, has anyone else expressed interest in using this
> feature?

Yes, in the earlier threads, Mimi spoke up in favor of it as a
possible path for IMA to do signature checking. She sent patches that
updated the LSM hooks to include callback to IMA that were sent to the
lsm list:
http://marc.info/?l=linux-security-module&m=134739023306344&w=2

Serge and Eric both Acked the new hooks too.

-Kees

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Mimi Zohar]

On Fri, 2012-09-21 at 12:22 +1000, James Morris wrote:
> On Thu, 20 Sep 2012, Kees Cook wrote:
> 
> > 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.
> 
> Just out of interest, has anyone else expressed interest in using this 
> feature?

I'm not so interested in this particular use case, but am interested in
using the new syscall's file descriptor for measuring/appraising a
kernel module's integrity.

thanks,

Mimi

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

[Mimi Zohar]

On Thu, 2012-09-20 at 15:14 -0700, 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 <keescook@chromium.org>
> Acked-by: Serge E. Hallyn <serge.hallyn@canonical.com>
> Acked-by: Eric Paris <eparis@redhat.com>

Thanks, please include my
Acked-by: Mimi Zohar <zohar@us.ibm.com>

> ---
>  include/linux/security.h |   13 +++++++++++++
>  kernel/module.c          |    9 +++++++++
>  security/capability.c    |    6 ++++++
>  security/security.c      |    5 +++++
>  4 files changed, 33 insertions(+), 0 deletions(-)
> 
> 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 afe2f69..511b8e9 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>
> @@ -2430,6 +2431,10 @@ static int copy_module_from_user(const void __user *umod, unsigned long 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)
> @@ -2464,6 +2469,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)
>  {

Re: [PATCH 3/4] ARM: add finit_module syscall to ARM

[Arnd Bergmann]

On Thursday 20 September 2012, Kees Cook wrote:
> Add finit_module syscall to the ARM syscall list.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---

My Ack was on the asm-generic patch, not the ARM architecture
one. This patch looks ok as well, but Russell is the one
maintaining the file, so you should get him to Ack this patch
instead.

	Arnd

Re: [PATCH 3/4] ARM: add finit_module syscall to ARM

[Russell King]

On Fri, Sep 21, 2012 at 01:15:06PM +0000, Arnd Bergmann wrote:
> On Thursday 20 September 2012, Kees Cook wrote:
> > Add finit_module syscall to the ARM syscall list.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> 
> My Ack was on the asm-generic patch, not the ARM architecture
> one. This patch looks ok as well, but Russell is the one
> maintaining the file, so you should get him to Ack this patch
> instead.

And, as no one bothered to copy me, I can't give an ack for a patch I've
not seen.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 3/4] ARM: add finit_module syscall to ARM

[Kees Cook]

On Fri, Sep 21, 2012 at 7:59 AM, Russell King <rmk@arm.linux.org.uk> wrote:
> On Fri, Sep 21, 2012 at 01:15:06PM +0000, Arnd Bergmann wrote:
>> On Thursday 20 September 2012, Kees Cook wrote:
>> > Add finit_module syscall to the ARM syscall list.
>> >
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> > Acked-by: Arnd Bergmann <arnd@arndb.de>
>> > ---
>>
>> My Ack was on the asm-generic patch, not the ARM architecture
>> one. This patch looks ok as well, but Russell is the one
>> maintaining the file, so you should get him to Ack this patch
>> instead.
>
> And, as no one bothered to copy me, I can't give an ack for a patch I've
> not seen.

Arg, sorry for this glitch on both counts. I'll fix these and re-send.

-Kees

-- 
Kees Cook
Chrome OS Security

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

[John Johansen]

On 09/20/2012 07:22 PM, James Morris wrote:
> On Thu, 20 Sep 2012, Kees Cook wrote:
> 
>> 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.
> 
> Just out of interest, has anyone else expressed interest in using this 
> feature?
> 
we are looking at using it in apparmor as well

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

[Kees Cook]

On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook <keescook@chromium.org> wrote:
> 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.

Hi Rusty,

Is this likely to land in the 3.7 change window? I'd really like to
get the syscall number assigned so I can start sending patches to
glibc, kmod, etc. My tree is here, FWIW:

http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Rusty Russell]

Kees Cook <keescook@chromium.org> writes:

> On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook <keescook@chromium.org> wrote:
>> 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.
>
> Hi Rusty,
>
> Is this likely to land in the 3.7 change window? I'd really like to
> get the syscall number assigned so I can start sending patches to
> glibc, kmod, etc. My tree is here, FWIW:

No, unfortunately it's a little late and there were issues with ARM
signoffs and syscall numbers...

> http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Messy merge due to the module signing stuff going in :(

Please rebase on top of my kernel.org modules-next branch, and I'll pull
into my modules-wip branch for 3.8.

Thanks,
Rusty.

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

[Mimi Zohar]

On Thu, 2012-10-04 at 15:09 +0930, Rusty Russell wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook <keescook@chromium.org> wrote:
> >> 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.
> >
> > Hi Rusty,
> >
> > Is this likely to land in the 3.7 change window? I'd really like to
> > get the syscall number assigned so I can start sending patches to
> > glibc, kmod, etc. My tree is here, FWIW:
> 
> No, unfortunately it's a little late and there were issues with ARM
> signoffs and syscall numbers...
> 
> > http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall
> 
> Messy merge due to the module signing stuff going in :(
> 
> Please rebase on top of my kernel.org modules-next branch, and I'll pull
> into my modules-wip branch for 3.8.

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!

Mimi

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

[Kees Cook]

On Wed, Oct 3, 2012 at 10:39 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 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.
>>
>> Hi Rusty,
>>
>> Is this likely to land in the 3.7 change window? I'd really like to
>> get the syscall number assigned so I can start sending patches to
>> glibc, kmod, etc. My tree is here, FWIW:
>
> No, unfortunately it's a little late and there were issues with ARM
> signoffs and syscall numbers...
>
>> http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall
>
> Messy merge due to the module signing stuff going in :(

Sure was! :) I've done the merge now (and sent the v5 patches). I
think it looks pretty clean now.

> Please rebase on top of my kernel.org modules-next branch, and I'll pull
> into my modules-wip branch for 3.8.

As Mimi mentioned, it would be really nice if this could land in 3.7.
Can I maybe convince you? It's technically a small change, just with a
lot of reordering of the calling code, but I think it's a relatively
small change. The diff output is horrible due to extracting
do_init_module, but the code changed is pretty minimal.

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Rusty Russell]

Mimi Zohar <zohar@linux.vnet.ibm.com> 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.

Cheers,
Rusty.

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

[Kees Cook]

On Thu, Oct 4, 2012 at 8:50 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> 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

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

[Michael Kerrisk]

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.

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

[H. Peter Anvin]

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

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

[Michael Kerrisk (man-pages)]

On Tue, Oct 9, 2012 at 11:58 PM, H. Peter Anvin <hpa@zytor.com> 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/

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

[H. Peter Anvin]

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

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

[Michael Kerrisk (man-pages)]

[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" <hpa@zytor.com> 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).

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

[Rusty Russell]

"H. Peter Anvin" <hpa@zytor.com> 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.

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

[Michael Kerrisk (man-pages)]

Rusty,

On Fri, Oct 12, 2012 at 12:16 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "H. Peter Anvin" <hpa@zytor.com> 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

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

[Rusty Russell]

"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> 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)

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

[H. Peter Anvin]

On 10/11/2012 03:16 PM, Rusty Russell wrote:
> "H. Peter Anvin" <hpa@zytor.com> 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

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

[Lucas De Marchi]

On Thu, Oct 18, 2012 at 12:12 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> 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

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

[Michael Kerrisk (man-pages)]

On Thu, Oct 18, 2012 at 6:24 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 10/11/2012 03:16 PM, Rusty Russell wrote:
>> "H. Peter Anvin" <hpa@zytor.com> 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/

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

[Michael Kerrisk (man-pages)]

On Thu, Oct 18, 2012 at 5:12 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> 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 <mtk.manpages@gmail.com>

> +       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/

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

[H. Peter Anvin]

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.

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

[Kees Cook]

On Thu, Oct 18, 2012 at 7:26 AM, H. Peter Anvin <hpa@zytor.com> 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

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

[H. Peter Anvin]

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.

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

[Rusty Russell]

"H. Peter Anvin" <hpa@zytor.com> 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.

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

[H. Peter Anvin]

On 10/18/2012 07:23 PM, Rusty Russell wrote:
> "H. Peter Anvin" <hpa@zytor.com> 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.

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

[Alon Ziv]

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

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

[Rusty Russell]

"H. Peter Anvin" <hpa@zytor.com> writes:
> On 10/18/2012 07:23 PM, Rusty Russell wrote:
>> "H. Peter Anvin" <hpa@zytor.com> 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.

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

[Rusty Russell]

"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>> FIX: add flags arg to sys_finit_module()
>>
>> Thanks to Michael Kerrisk for keeping us honest.
>
> w00t! Thanks, Rusty ;-).
>
> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>

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 <rusty@rustcorp.com.au>

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)

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

[Lucas De Marchi]

On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>
> 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 <rusty@rustcorp.com.au>
>
> 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 <lucas.demarchi@profusion.mobi>

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

[Kees Cook]

On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>>> FIX: add flags arg to sys_finit_module()
>>>>
>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>
>>> w00t! Thanks, Rusty ;-).
>>>
>>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>>
>> 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 <rusty@rustcorp.com.au>

Acked-by: Kees Cook <keescook@chromium.org>

> 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

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

[Lucas De Marchi]

On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>>>> FIX: add flags arg to sys_finit_module()
>>>>>
>>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>>
>>>> w00t! Thanks, Rusty ;-).
>>>>
>>>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>>>
>>> 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 <rusty@rustcorp.com.au>
>
> Acked-by: Kees Cook <keescook@chromium.org>
>
>> 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

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

[Michael Kerrisk (man-pages)]

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>
> 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/

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

[Kees Cook]

On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>> <lucas.demarchi@profusion.mobi> wrote:
>>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>>>>> FIX: add flags arg to sys_finit_module()
>>>>>>
>>>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>>>
>>>>> w00t! Thanks, Rusty ;-).
>>>>>
>>>>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>>>>
>>>> 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 <rusty@rustcorp.com.au>
>>
>> Acked-by: Kees Cook <keescook@chromium.org>
>>
>>> 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

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

[H. Peter Anvin]

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.

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

[Lucas De Marchi]

On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook <keescook@chromium.org> wrote:
>>> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>>> <lucas.demarchi@profusion.mobi> wrote:
>>>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>>> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>>>>>> FIX: add flags arg to sys_finit_module()
>>>>>>>
>>>>>>> Thanks to Michael Kerrisk for keeping us honest.
>>>>>>
>>>>>> w00t! Thanks, Rusty ;-).
>>>>>>
>>>>>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>>>>>
>>>>> 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 <rusty@rustcorp.com.au>
>>>
>>> Acked-by: Kees Cook <keescook@chromium.org>
>>>
>>>> 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

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

[Rusty Russell]

Lucas De Marchi <lucas.demarchi@profusion.mobi> writes:
> On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
>> <lucas.demarchi@profusion.mobi> 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.

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

[Kees Cook]

On Mon, Oct 22, 2012 at 12:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>
> 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 <rusty@rustcorp.com.au>

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 <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Rusty Russell]

Kees Cook <keescook@chromium.org> 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.

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

[Michael Kerrisk]

Hi Rusty,

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
>
> 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.

The description here is rather thin. Could you supply a sentence or
two for each of MODULE_INIT_IGNORE_MODVERSIONS and
MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
page?

Thanks,

Michael


>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> 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)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



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

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

[Michael Kerrisk]

Hi Rusty,

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
> "Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> 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.

The description here is rather thin. Could you supply a sentence or
two for each of MODULE_INIT_IGNORE_MODVERSIONS and
MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
page?

Thanks,

Michael


>
> Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
>
> 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)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rusty Russell]

Michael Kerrisk <mtk.manpages@gmail.com> writes:
> Hi Rusty,

Hi Michael,

> The description here is rather thin. Could you supply a sentence or
> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
> page?
>
> Thanks,

There are one or two safety checks built into a module, which are
checked to match the kernel on module load.  The first is a "vermagic"
string containing the kernel version number and prominent features (such
as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
version hash is recorded for each symbol the module uses based on the
types it refers to: in this case, the kernel version number within the
"vermagic" string is ignored, as the symbol version hashes are assumed
to be sufficiently reliable.

Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
that the version hashes are to be ignored.  If the kernel is built to
permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
loading will continue, otherwise it will fail with ENOEXEC as expected
for malformed modules.

Hope that is more usable?

Thanks,
Rusty,

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

[Rusty Russell]

Michael Kerrisk <mtk.manpages@gmail.com> writes:
> Hi Rusty,

Hi Michael,

> The description here is rather thin. Could you supply a sentence or
> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
> page?
>
> Thanks,

There are one or two safety checks built into a module, which are
checked to match the kernel on module load.  The first is a "vermagic"
string containing the kernel version number and prominent features (such
as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
version hash is recorded for each symbol the module uses based on the
types it refers to: in this case, the kernel version number within the
"vermagic" string is ignored, as the symbol version hashes are assumed
to be sufficiently reliable.

Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
that the version hashes are to be ignored.  If the kernel is built to
permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
loading will continue, otherwise it will fail with ENOEXEC as expected
for malformed modules.

Hope that is more usable?

Thanks,
Rusty,

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

[Michael Kerrisk (man-pages)]

Hi Rusty, (and Lucas, and Kees)

On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Michael Kerrisk <mtk.manpages@gmail.com> writes:
>> Hi Rusty,
>
> Hi Michael,
>
>> The description here is rather thin. Could you supply a sentence or
>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>> page?
>>
>> Thanks,
>
> There are one or two safety checks built into a module, which are
> checked to match the kernel on module load.  The first is a "vermagic"
> string containing the kernel version number and prominent features (such
> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
> version hash is recorded for each symbol the module uses based on the
> types it refers to: in this case, the kernel version number within the
> "vermagic" string is ignored, as the symbol version hashes are assumed
> to be sufficiently reliable.
>
> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
> that the version hashes are to be ignored.  If the kernel is built to
> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
> loading will continue, otherwise it will fail with ENOEXEC as expected
> for malformed modules.
>
> Hope that is more usable?

Yes, that helps. I did some reworking of that text. Hopefully, I did
not introduce any errors.

Below is the text that is proposed to document finit_module() in the
man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Thanks,

Michael

   finit_module()
       The finit_module() system call is like init_module(), but reads
       the module to be loaded from the file  descriptor  fd.   It  is
       useful  when  the authenticity of a kernel module can be deter‐
       mined from its location in the file system; in cases where that
       is  possible,  the  overhead  of using cryptographically signed
       modules to determine  the  authenticity  of  a  module  can  be
       avoided.  The param_values argument is as for init_module().

       The  flags  argument  modifies the operation of finit_module().
       It is a bit mask value created by ORing together zero  or  more
       of the following flags:

       MODULE_INIT_IGNORE_MODVERSIONS
              Ignore symbol version hashes.

       MODULE_INIT_IGNORE_VERMAGIC
              Ignore kernel version magic.

       There are some safety checks built into a module to ensure that
       it matches the kernel against which it is loaded.  These checks
       are  recorded  when  the  module is built and verified when the
       module is loaded.   First,  the  module  records  a  "vermagic"
       string  containing the kernel version number and prominent fea‐
       tures (such as the CPU type).  Second, if the module was  built
       with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
       version hash is recorded for each symbol the module uses.  This
       hash  is  based  on the types of the arguments and return value
       for the function named by the symbol.  In this case, the kernel
       version  number within the "vermagic" string is ignored, as the
       symbol version hashes are assumed to be sufficiently reliable.

       Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
       "vermagic"   string   is   to   be   ignored,   and   the  MOD‐
       ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
       sion  hashes are to be ignored.  If the kernel is built to per‐
       mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
       ULE_FORCE_LOAD),  then loading will continue, otherwise it will
       fail with ENOEXEC as expected for malformed modules.
...
   ERRORS
...
       The following errors may additionally occur for finit_module():

       EBADF  The file referred to by fd is not opened for reading.

       EFBIG  The file referred to by fd is too large.

       EINVAL flags is invalid.

       ENOEXEC
              fd does not refer to an open file.


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

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

[Michael Kerrisk (man-pages)]

Hi Rusty, (and Lucas, and Kees)

On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
> Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> Hi Rusty,
>
> Hi Michael,
>
>> The description here is rather thin. Could you supply a sentence or
>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>> page?
>>
>> Thanks,
>
> There are one or two safety checks built into a module, which are
> checked to match the kernel on module load.  The first is a "vermagic"
> string containing the kernel version number and prominent features (such
> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
> version hash is recorded for each symbol the module uses based on the
> types it refers to: in this case, the kernel version number within the
> "vermagic" string is ignored, as the symbol version hashes are assumed
> to be sufficiently reliable.
>
> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
> that the version hashes are to be ignored.  If the kernel is built to
> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
> loading will continue, otherwise it will fail with ENOEXEC as expected
> for malformed modules.
>
> Hope that is more usable?

Yes, that helps. I did some reworking of that text. Hopefully, I did
not introduce any errors.

Below is the text that is proposed to document finit_module() in the
man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Thanks,

Michael

   finit_module()
       The finit_module() system call is like init_module(), but reads
       the module to be loaded from the file  descriptor  fd.   It  is
       useful  when  the authenticity of a kernel module can be deter‐
       mined from its location in the file system; in cases where that
       is  possible,  the  overhead  of using cryptographically signed
       modules to determine  the  authenticity  of  a  module  can  be
       avoided.  The param_values argument is as for init_module().

       The  flags  argument  modifies the operation of finit_module().
       It is a bit mask value created by ORing together zero  or  more
       of the following flags:

       MODULE_INIT_IGNORE_MODVERSIONS
              Ignore symbol version hashes.

       MODULE_INIT_IGNORE_VERMAGIC
              Ignore kernel version magic.

       There are some safety checks built into a module to ensure that
       it matches the kernel against which it is loaded.  These checks
       are  recorded  when  the  module is built and verified when the
       module is loaded.   First,  the  module  records  a  "vermagic"
       string  containing the kernel version number and prominent fea‐
       tures (such as the CPU type).  Second, if the module was  built
       with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
       version hash is recorded for each symbol the module uses.  This
       hash  is  based  on the types of the arguments and return value
       for the function named by the symbol.  In this case, the kernel
       version  number within the "vermagic" string is ignored, as the
       symbol version hashes are assumed to be sufficiently reliable.

       Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
       "vermagic"   string   is   to   be   ignored,   and   the  MOD‐
       ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
       sion  hashes are to be ignored.  If the kernel is built to per‐
       mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
       ULE_FORCE_LOAD),  then loading will continue, otherwise it will
       fail with ENOEXEC as expected for malformed modules.
...
   ERRORS
...
       The following errors may additionally occur for finit_module():

       EBADF  The file referred to by fd is not opened for reading.

       EFBIG  The file referred to by fd is too large.

       EINVAL flags is invalid.

       ENOEXEC
              fd does not refer to an open file.


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kees Cook]

On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> Hi Rusty, (and Lucas, and Kees)
>
> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Michael Kerrisk <mtk.manpages@gmail.com> writes:
>>> Hi Rusty,
>>
>> Hi Michael,
>>
>>> The description here is rather thin. Could you supply a sentence or
>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>> page?
>>>
>>> Thanks,
>>
>> There are one or two safety checks built into a module, which are
>> checked to match the kernel on module load.  The first is a "vermagic"
>> string containing the kernel version number and prominent features (such
>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>> version hash is recorded for each symbol the module uses based on the
>> types it refers to: in this case, the kernel version number within the
>> "vermagic" string is ignored, as the symbol version hashes are assumed
>> to be sufficiently reliable.
>>
>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>> that the version hashes are to be ignored.  If the kernel is built to
>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>> loading will continue, otherwise it will fail with ENOEXEC as expected
>> for malformed modules.
>>
>> Hope that is more usable?
>
> Yes, that helps. I did some reworking of that text. Hopefully, I did
> not introduce any errors.
>
> Below is the text that is proposed to document finit_module() in the
> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Looks good to me!

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

--
Kees Cook
Chrome OS Security

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

[Kees Cook]

On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
<mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Rusty, (and Lucas, and Kees)
>
> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
>> Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>> Hi Rusty,
>>
>> Hi Michael,
>>
>>> The description here is rather thin. Could you supply a sentence or
>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>> page?
>>>
>>> Thanks,
>>
>> There are one or two safety checks built into a module, which are
>> checked to match the kernel on module load.  The first is a "vermagic"
>> string containing the kernel version number and prominent features (such
>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>> version hash is recorded for each symbol the module uses based on the
>> types it refers to: in this case, the kernel version number within the
>> "vermagic" string is ignored, as the symbol version hashes are assumed
>> to be sufficiently reliable.
>>
>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>> that the version hashes are to be ignored.  If the kernel is built to
>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>> loading will continue, otherwise it will fail with ENOEXEC as expected
>> for malformed modules.
>>
>> Hope that is more usable?
>
> Yes, that helps. I did some reworking of that text. Hopefully, I did
> not introduce any errors.
>
> Below is the text that is proposed to document finit_module() in the
> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Looks good to me!

Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Michael Kerrisk (man-pages)]

On Sun, Jan 6, 2013 at 9:24 PM, Kees Cook <keescook@chromium.org> wrote:
> On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
>> Hi Rusty, (and Lucas, and Kees)
>>
>> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Michael Kerrisk <mtk.manpages@gmail.com> writes:
>>>> Hi Rusty,
>>>
>>> Hi Michael,
>>>
>>>> The description here is rather thin. Could you supply a sentence or
>>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>>> page?
>>>>
>>>> Thanks,
>>>
>>> There are one or two safety checks built into a module, which are
>>> checked to match the kernel on module load.  The first is a "vermagic"
>>> string containing the kernel version number and prominent features (such
>>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>>> version hash is recorded for each symbol the module uses based on the
>>> types it refers to: in this case, the kernel version number within the
>>> "vermagic" string is ignored, as the symbol version hashes are assumed
>>> to be sufficiently reliable.
>>>
>>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>>> that the version hashes are to be ignored.  If the kernel is built to
>>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>>> loading will continue, otherwise it will fail with ENOEXEC as expected
>>> for malformed modules.
>>>
>>> Hope that is more usable?
>>
>> Yes, that helps. I did some reworking of that text. Hopefully, I did
>> not introduce any errors.
>>
>> Below is the text that is proposed to document finit_module() in the
>> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)
>
> Looks good to me!
>
> Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks Kees!



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

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

[Michael Kerrisk (man-pages)]

On Sun, Jan 6, 2013 at 9:24 PM, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
> <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi Rusty, (and Lucas, and Kees)
>>
>> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
>>> Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>>> Hi Rusty,
>>>
>>> Hi Michael,
>>>
>>>> The description here is rather thin. Could you supply a sentence or
>>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>>> page?
>>>>
>>>> Thanks,
>>>
>>> There are one or two safety checks built into a module, which are
>>> checked to match the kernel on module load.  The first is a "vermagic"
>>> string containing the kernel version number and prominent features (such
>>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>>> version hash is recorded for each symbol the module uses based on the
>>> types it refers to: in this case, the kernel version number within the
>>> "vermagic" string is ignored, as the symbol version hashes are assumed
>>> to be sufficiently reliable.
>>>
>>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>>> that the version hashes are to be ignored.  If the kernel is built to
>>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>>> loading will continue, otherwise it will fail with ENOEXEC as expected
>>> for malformed modules.
>>>
>>> Hope that is more usable?
>>
>> Yes, that helps. I did some reworking of that text. Hopefully, I did
>> not introduce any errors.
>>
>> Below is the text that is proposed to document finit_module() in the
>> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)
>
> Looks good to me!
>
> Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Thanks Kees!



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Lucas De Marchi]

On Sun, Jan 6, 2013 at 4:59 PM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> Hi Rusty, (and Lucas, and Kees)
>
> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Michael Kerrisk <mtk.manpages@gmail.com> writes:
>>> Hi Rusty,
>>
>> Hi Michael,
>>
>>> The description here is rather thin. Could you supply a sentence or
>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>> page?
>>>
>>> Thanks,
>>
>> There are one or two safety checks built into a module, which are
>> checked to match the kernel on module load.  The first is a "vermagic"
>> string containing the kernel version number and prominent features (such
>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>> version hash is recorded for each symbol the module uses based on the
>> types it refers to: in this case, the kernel version number within the
>> "vermagic" string is ignored, as the symbol version hashes are assumed
>> to be sufficiently reliable.
>>
>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>> that the version hashes are to be ignored.  If the kernel is built to
>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>> loading will continue, otherwise it will fail with ENOEXEC as expected
>> for malformed modules.
>>
>> Hope that is more usable?
>
> Yes, that helps. I did some reworking of that text. Hopefully, I did
> not introduce any errors.
>
> Below is the text that is proposed to document finit_module() in the
> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)
>
> Thanks,
>
> Michael
>
>    finit_module()
>        The finit_module() system call is like init_module(), but reads
>        the module to be loaded from the file  descriptor  fd.   It  is
>        useful  when  the authenticity of a kernel module can be deter‐
>        mined from its location in the file system; in cases where that
>        is  possible,  the  overhead  of using cryptographically signed
>        modules to determine  the  authenticity  of  a  module  can  be
>        avoided.  The param_values argument is as for init_module().
>
>        The  flags  argument  modifies the operation of finit_module().
>        It is a bit mask value created by ORing together zero  or  more
>        of the following flags:
>
>        MODULE_INIT_IGNORE_MODVERSIONS
>               Ignore symbol version hashes.
>
>        MODULE_INIT_IGNORE_VERMAGIC
>               Ignore kernel version magic.
>
>        There are some safety checks built into a module to ensure that
>        it matches the kernel against which it is loaded.  These checks
>        are  recorded  when  the  module is built and verified when the
>        module is loaded.   First,  the  module  records  a  "vermagic"
>        string  containing the kernel version number and prominent fea‐
>        tures (such as the CPU type).  Second, if the module was  built
>        with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
>        version hash is recorded for each symbol the module uses.  This
>        hash  is  based  on the types of the arguments and return value
>        for the function named by the symbol.  In this case, the kernel
>        version  number within the "vermagic" string is ignored, as the
>        symbol version hashes are assumed to be sufficiently reliable.
>
>        Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
>        "vermagic"   string   is   to   be   ignored,   and   the  MOD‐
>        ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
>        sion  hashes are to be ignored.  If the kernel is built to per‐
>        mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
>        ULE_FORCE_LOAD),  then loading will continue, otherwise it will
>        fail with ENOEXEC as expected for malformed modules.
> ...
>    ERRORS
> ...
>        The following errors may additionally occur for finit_module():
>
>        EBADF  The file referred to by fd is not opened for reading.
>
>        EFBIG  The file referred to by fd is too large.
>
>        EINVAL flags is invalid.
>
>        ENOEXEC
>               fd does not refer to an open file.
>
>


Looks good to me.


regards,
Lucas De Marchi

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

[Lucas De Marchi]

On Sun, Jan 6, 2013 at 4:59 PM, Michael Kerrisk (man-pages)
<mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Rusty, (and Lucas, and Kees)
>
> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
>> Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>> Hi Rusty,
>>
>> Hi Michael,
>>
>>> The description here is rather thin. Could you supply a sentence or
>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>> page?
>>>
>>> Thanks,
>>
>> There are one or two safety checks built into a module, which are
>> checked to match the kernel on module load.  The first is a "vermagic"
>> string containing the kernel version number and prominent features (such
>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>> version hash is recorded for each symbol the module uses based on the
>> types it refers to: in this case, the kernel version number within the
>> "vermagic" string is ignored, as the symbol version hashes are assumed
>> to be sufficiently reliable.
>>
>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>> that the version hashes are to be ignored.  If the kernel is built to
>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>> loading will continue, otherwise it will fail with ENOEXEC as expected
>> for malformed modules.
>>
>> Hope that is more usable?
>
> Yes, that helps. I did some reworking of that text. Hopefully, I did
> not introduce any errors.
>
> Below is the text that is proposed to document finit_module() in the
> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)
>
> Thanks,
>
> Michael
>
>    finit_module()
>        The finit_module() system call is like init_module(), but reads
>        the module to be loaded from the file  descriptor  fd.   It  is
>        useful  when  the authenticity of a kernel module can be deter‐
>        mined from its location in the file system; in cases where that
>        is  possible,  the  overhead  of using cryptographically signed
>        modules to determine  the  authenticity  of  a  module  can  be
>        avoided.  The param_values argument is as for init_module().
>
>        The  flags  argument  modifies the operation of finit_module().
>        It is a bit mask value created by ORing together zero  or  more
>        of the following flags:
>
>        MODULE_INIT_IGNORE_MODVERSIONS
>               Ignore symbol version hashes.
>
>        MODULE_INIT_IGNORE_VERMAGIC
>               Ignore kernel version magic.
>
>        There are some safety checks built into a module to ensure that
>        it matches the kernel against which it is loaded.  These checks
>        are  recorded  when  the  module is built and verified when the
>        module is loaded.   First,  the  module  records  a  "vermagic"
>        string  containing the kernel version number and prominent fea‐
>        tures (such as the CPU type).  Second, if the module was  built
>        with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
>        version hash is recorded for each symbol the module uses.  This
>        hash  is  based  on the types of the arguments and return value
>        for the function named by the symbol.  In this case, the kernel
>        version  number within the "vermagic" string is ignored, as the
>        symbol version hashes are assumed to be sufficiently reliable.
>
>        Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
>        "vermagic"   string   is   to   be   ignored,   and   the  MOD‐
>        ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
>        sion  hashes are to be ignored.  If the kernel is built to per‐
>        mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
>        ULE_FORCE_LOAD),  then loading will continue, otherwise it will
>        fail with ENOEXEC as expected for malformed modules.
> ...
>    ERRORS
> ...
>        The following errors may additionally occur for finit_module():
>
>        EBADF  The file referred to by fd is not opened for reading.
>
>        EFBIG  The file referred to by fd is too large.
>
>        EINVAL flags is invalid.
>
>        ENOEXEC
>               fd does not refer to an open file.
>
>


Looks good to me.


regards,
Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Michael Kerrisk (man-pages)]

On Wed, Jan 9, 2013 at 6:29 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Sun, Jan 6, 2013 at 4:59 PM, Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
>> Hi Rusty, (and Lucas, and Kees)
>>
>> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Michael Kerrisk <mtk.manpages@gmail.com> writes:
>>>> Hi Rusty,
>>>
>>> Hi Michael,
>>>
>>>> The description here is rather thin. Could you supply a sentence or
>>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>>> page?
>>>>
>>>> Thanks,
>>>
>>> There are one or two safety checks built into a module, which are
>>> checked to match the kernel on module load.  The first is a "vermagic"
>>> string containing the kernel version number and prominent features (such
>>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>>> version hash is recorded for each symbol the module uses based on the
>>> types it refers to: in this case, the kernel version number within the
>>> "vermagic" string is ignored, as the symbol version hashes are assumed
>>> to be sufficiently reliable.
>>>
>>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>>> that the version hashes are to be ignored.  If the kernel is built to
>>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>>> loading will continue, otherwise it will fail with ENOEXEC as expected
>>> for malformed modules.
>>>
>>> Hope that is more usable?
>>
>> Yes, that helps. I did some reworking of that text. Hopefully, I did
>> not introduce any errors.
>>
>> Below is the text that is proposed to document finit_module() in the
>> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)
>>
>> Thanks,
>>
>> Michael
>>
>>    finit_module()
>>        The finit_module() system call is like init_module(), but reads
>>        the module to be loaded from the file  descriptor  fd.   It  is
>>        useful  when  the authenticity of a kernel module can be deter‐
>>        mined from its location in the file system; in cases where that
>>        is  possible,  the  overhead  of using cryptographically signed
>>        modules to determine  the  authenticity  of  a  module  can  be
>>        avoided.  The param_values argument is as for init_module().
>>
>>        The  flags  argument  modifies the operation of finit_module().
>>        It is a bit mask value created by ORing together zero  or  more
>>        of the following flags:
>>
>>        MODULE_INIT_IGNORE_MODVERSIONS
>>               Ignore symbol version hashes.
>>
>>        MODULE_INIT_IGNORE_VERMAGIC
>>               Ignore kernel version magic.
>>
>>        There are some safety checks built into a module to ensure that
>>        it matches the kernel against which it is loaded.  These checks
>>        are  recorded  when  the  module is built and verified when the
>>        module is loaded.   First,  the  module  records  a  "vermagic"
>>        string  containing the kernel version number and prominent fea‐
>>        tures (such as the CPU type).  Second, if the module was  built
>>        with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
>>        version hash is recorded for each symbol the module uses.  This
>>        hash  is  based  on the types of the arguments and return value
>>        for the function named by the symbol.  In this case, the kernel
>>        version  number within the "vermagic" string is ignored, as the
>>        symbol version hashes are assumed to be sufficiently reliable.
>>
>>        Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
>>        "vermagic"   string   is   to   be   ignored,   and   the  MOD‐
>>        ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
>>        sion  hashes are to be ignored.  If the kernel is built to per‐
>>        mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
>>        ULE_FORCE_LOAD),  then loading will continue, otherwise it will
>>        fail with ENOEXEC as expected for malformed modules.
>> ...
>>    ERRORS
>> ...
>>        The following errors may additionally occur for finit_module():
>>
>>        EBADF  The file referred to by fd is not opened for reading.
>>
>>        EFBIG  The file referred to by fd is too large.
>>
>>        EINVAL flags is invalid.
>>
>>        ENOEXEC
>>               fd does not refer to an open file.
>>
>>
>
>
> Looks good to me.

Thanks for looking it over, Lucas.

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/

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

[Michael Kerrisk (man-pages)]

On Wed, Jan 9, 2013 at 6:29 PM, Lucas De Marchi
<lucas.demarchi-Y3ZbgMPKUGA34EUeqzHoZw@public.gmane.org> wrote:
> On Sun, Jan 6, 2013 at 4:59 PM, Michael Kerrisk (man-pages)
> <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi Rusty, (and Lucas, and Kees)
>>
>> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
>>> Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>>> Hi Rusty,
>>>
>>> Hi Michael,
>>>
>>>> The description here is rather thin. Could you supply a sentence or
>>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>>> page?
>>>>
>>>> Thanks,
>>>
>>> There are one or two safety checks built into a module, which are
>>> checked to match the kernel on module load.  The first is a "vermagic"
>>> string containing the kernel version number and prominent features (such
>>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>>> version hash is recorded for each symbol the module uses based on the
>>> types it refers to: in this case, the kernel version number within the
>>> "vermagic" string is ignored, as the symbol version hashes are assumed
>>> to be sufficiently reliable.
>>>
>>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>>> that the version hashes are to be ignored.  If the kernel is built to
>>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>>> loading will continue, otherwise it will fail with ENOEXEC as expected
>>> for malformed modules.
>>>
>>> Hope that is more usable?
>>
>> Yes, that helps. I did some reworking of that text. Hopefully, I did
>> not introduce any errors.
>>
>> Below is the text that is proposed to document finit_module() in the
>> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)
>>
>> Thanks,
>>
>> Michael
>>
>>    finit_module()
>>        The finit_module() system call is like init_module(), but reads
>>        the module to be loaded from the file  descriptor  fd.   It  is
>>        useful  when  the authenticity of a kernel module can be deter‐
>>        mined from its location in the file system; in cases where that
>>        is  possible,  the  overhead  of using cryptographically signed
>>        modules to determine  the  authenticity  of  a  module  can  be
>>        avoided.  The param_values argument is as for init_module().
>>
>>        The  flags  argument  modifies the operation of finit_module().
>>        It is a bit mask value created by ORing together zero  or  more
>>        of the following flags:
>>
>>        MODULE_INIT_IGNORE_MODVERSIONS
>>               Ignore symbol version hashes.
>>
>>        MODULE_INIT_IGNORE_VERMAGIC
>>               Ignore kernel version magic.
>>
>>        There are some safety checks built into a module to ensure that
>>        it matches the kernel against which it is loaded.  These checks
>>        are  recorded  when  the  module is built and verified when the
>>        module is loaded.   First,  the  module  records  a  "vermagic"
>>        string  containing the kernel version number and prominent fea‐
>>        tures (such as the CPU type).  Second, if the module was  built
>>        with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
>>        version hash is recorded for each symbol the module uses.  This
>>        hash  is  based  on the types of the arguments and return value
>>        for the function named by the symbol.  In this case, the kernel
>>        version  number within the "vermagic" string is ignored, as the
>>        symbol version hashes are assumed to be sufficiently reliable.
>>
>>        Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
>>        "vermagic"   string   is   to   be   ignored,   and   the  MOD‐
>>        ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
>>        sion  hashes are to be ignored.  If the kernel is built to per‐
>>        mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
>>        ULE_FORCE_LOAD),  then loading will continue, otherwise it will
>>        fail with ENOEXEC as expected for malformed modules.
>> ...
>>    ERRORS
>> ...
>>        The following errors may additionally occur for finit_module():
>>
>>        EBADF  The file referred to by fd is not opened for reading.
>>
>>        EFBIG  The file referred to by fd is too large.
>>
>>        EINVAL flags is invalid.
>>
>>        ENOEXEC
>>               fd does not refer to an open file.
>>
>>
>
>
> Looks good to me.

Thanks for looking it over, Lucas.

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/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kees Cook]

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 <keescook@chromium.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
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