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

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

[Kees Cook]

Instead of (or in addition to) kernel module signing, being able to reason
about the origin of a kernel module would be valuable in situations
where an OS already trusts a specific file system, file, etc, due to
things like security labels or an existing root of trust to a partition
through things like dm-verity.

This introduces a new syscall (currently only 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>
---
 arch/x86/syscalls/syscall_32.tbl |    1 +
 arch/x86/syscalls/syscall_64.tbl |    1 +
 include/linux/syscalls.h         |    1 +
 kernel/module.c                  |  219 +++++++++++++++++++++++++++-----------
 kernel/sys_ni.c                  |    1 +
 5 files changed, 159 insertions(+), 64 deletions(-)

diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 7a35a6e..12ddc6e 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	init_module_from_fd	sys_init_module_from_fd
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index a582bfe..9b25734 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	init_module_from_fd	sys_init_module_from_fd
 
 #
 # 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..5386629 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_init_module_from_fd(int len, const char __user *uargs);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 4edbd9c..b080cf8 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,107 @@ 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)
+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) {
-		err = -EFAULT;
+	err = copy_from_user(info->hdr, umod, info->len);
+	if (err)
 		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. */
+int copy_module_from_fd(int fd, struct load_info *info)
+{
+	struct file *file;
+	int err;
+	struct kstat stat;
+	unsigned long size;
+	off_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 = -ENOMEM;
+		goto out;
 	}
+	size = stat.size;
 
-	if (hdr->e_shoff >= len ||
-	    hdr->e_shnum * sizeof(Elf_Shdr) > len - hdr->e_shoff) {
-		err = -ENOEXEC;
-		goto free_hdr;
+	info->hdr = vmalloc(size);
+	if (!info->hdr) {
+		err = -ENOMEM;
+		goto out;
 	}
 
-	info->hdr = hdr;
-	info->len = len;
-	return 0;
+	pos = 0;
+	while (pos < size) {
+		bytes = kernel_read(file, pos, (char *)(info->hdr) + pos,
+				    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);
+	err = check_info(info);
+	if (err)
+		vfree(info->hdr);
+
+out:
+	fput(file);
 	return err;
 }
 
@@ -2861,26 +2921,17 @@ static int post_relocation(struct module *mod, const struct load_info *info)
 	return module_finalize(info->hdr, info->sechdrs, mod);
 }
 
+static int do_init_module(struct module *mod);
+
 /* 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 +2944,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 +2992,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 +3010,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 +3029,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,10 +3041,10 @@ 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. */
@@ -3007,21 +3059,10 @@ static void do_mod_ctors(struct module *mod)
 }
 
 /* This is where the real work happens */
-SYSCALL_DEFINE3(init_module, void __user *, umod,
-		unsigned long, len, const char __user *, uargs)
+static int do_init_module(struct module *mod)
 {
-	struct module *mod;
 	int ret = 0;
 
-	/* Must have permission */
-	if (!capable(CAP_SYS_MODULE) || modules_disabled)
-		return -EPERM;
-
-	/* Do all the hard work */
-	mod = load_module(umod, len, uargs);
-	if (IS_ERR(mod))
-		return PTR_ERR(mod);
-
 	blocking_notifier_call_chain(&module_notify_list,
 			MODULE_STATE_COMING, mod);
 
@@ -3091,6 +3132,56 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 	return 0;
 }
 
+static int init_module_permission(void)
+{
+	/* Must have permission */
+	if (!capable(CAP_SYS_MODULE) || modules_disabled)
+		return -EPERM;
+
+	return 0;
+}
+
+SYSCALL_DEFINE2(init_module_from_fd, int, fd, const char __user *, uargs)
+{
+	int err;
+	struct load_info info = { };
+
+	err = init_module_permission();
+	if (err)
+		return err;
+
+	pr_debug("init_module_from_fd: fd=%d, uargs=%p\n", fd, uargs);
+
+	if (fd < 0)
+		return -ENOEXEC;
+
+	err = copy_module_from_fd(fd, &info);
+	if (err)
+		return err;
+
+	return load_module(&info, uargs);
+}
+
+SYSCALL_DEFINE3(init_module, void __user *, umod,
+		unsigned long, len, const char __user *, uargs)
+{
+	int err;
+	struct load_info info = { };
+
+	err = init_module_permission();
+	if (err)
+		return err;
+
+	pr_debug("init_module: umod=%p, len=%lu, uargs=%p\n",
+	       umod, len, uargs);
+
+	err = copy_module_from_user(umod, len, &info);
+	if (err)
+		return err;
+
+	return load_module(&info, uargs);
+}
+
 static inline int within(unsigned long addr, void *start, unsigned long size)
 {
 	return ((void *)addr >= start && (void *)addr < start + size);
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index dbff751..ed4a974 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_init_module_from_fd);
 cond_syscall(sys_delete_module);
 cond_syscall(sys_socketpair);
 cond_syscall(sys_bind);
-- 
1.7.0.4

[PATCH 2/2] 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.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Serge E. Hallyn <serge.hallyn@canonical.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 b080cf8..8a1aca1 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 @@ 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)
@@ -2474,6 +2479,10 @@ int copy_module_from_fd(int fd, struct load_info *info)
 	}
 	size = stat.size;
 
+	err = security_kernel_module_from_file(file);
+	if (err)
+		goto out;
+
 	info->hdr = vmalloc(size);
 	if (!info->hdr) {
 		err = -ENOMEM;
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

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

[Rusty Russell]

Kees Cook <keescook@chromium.org> writes:
> Instead of (or in addition to) kernel module signing, being able to reason
> about the origin of a kernel module would be valuable in situations
> where an OS already trusts a specific file system, file, etc, due to
> things like security labels or an existing root of trust to a partition
> through things like dm-verity.
>
> This introduces a new syscall (currently only 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.

Thanks.  Minor comments follow:

> +350	i386	init_module_from_fd	sys_init_module_from_fd

The from_ seems redundant.

> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 19439c7..5386629 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_init_module_from_fd(int len, const char __user *uargs);
>  #endif

You mean, "int fd"?

> diff --git a/kernel/module.c b/kernel/module.c
> index 4edbd9c..b080cf8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
...
> +/* Sets info->hdr and info->len. */
> +int copy_module_from_fd(int fd, struct load_info *info)
> +{
> +	struct file *file;
> +	int err;
> +	struct kstat stat;
> +	unsigned long size;
> +	off_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 = -ENOMEM;
> +		goto out;
>  	}
> +	size = stat.size;
>  
> -	if (hdr->e_shoff >= len ||
> -	    hdr->e_shnum * sizeof(Elf_Shdr) > len - hdr->e_shoff) {
> -		err = -ENOEXEC;
> -		goto free_hdr;
> +	info->hdr = vmalloc(size);
> +	if (!info->hdr) {
> +		err = -ENOMEM;
> +		goto out;

The stat.size > INT_MAX is redundant: vmalloc is quite careful on its
checking of the size param.

(We removed a similar test from the module.c code a few years ago).

Cheers,
Rusty.

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

[Kees Cook]

On Thu, Sep 6, 2012 at 5:15 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>> Instead of (or in addition to) kernel module signing, being able to reason
>> about the origin of a kernel module would be valuable in situations
>> where an OS already trusts a specific file system, file, etc, due to
>> things like security labels or an existing root of trust to a partition
>> through things like dm-verity.
>>
>> This introduces a new syscall (currently only 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.
>
> Thanks.  Minor comments follow:
>
>> +350  i386    init_module_from_fd     sys_init_module_from_fd
>
> The from_ seems redundant.

I'm happy to drop it.

>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index 19439c7..5386629 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_init_module_from_fd(int len, const char __user *uargs);
>>  #endif
>
> You mean, "int fd"?

Whoops. Thanks, I'll fix that.

>> diff --git a/kernel/module.c b/kernel/module.c
>> index 4edbd9c..b080cf8 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
> ...
>> +/* Sets info->hdr and info->len. */
>> +int copy_module_from_fd(int fd, struct load_info *info)
>> +{
>> +     struct file *file;
>> +     int err;
>> +     struct kstat stat;
>> +     unsigned long size;
>> +     off_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 = -ENOMEM;
>> +             goto out;
>>       }
>> +     size = stat.size;
>>
>> -     if (hdr->e_shoff >= len ||
>> -         hdr->e_shnum * sizeof(Elf_Shdr) > len - hdr->e_shoff) {
>> -             err = -ENOEXEC;
>> -             goto free_hdr;
>> +     info->hdr = vmalloc(size);
>> +     if (!info->hdr) {
>> +             err = -ENOMEM;
>> +             goto out;
>
> The stat.size > INT_MAX is redundant: vmalloc is quite careful on its
> checking of the size param.
>
> (We removed a similar test from the module.c code a few years ago).

Fair enough, I'll remove that too and send an updated version.

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Mimi Zohar]

On Fri, 2012-09-07 at 09:45 +0930, Rusty Russell wrote:
> Kees Cook <keescook@chromium.org> writes:
> > Instead of (or in addition to) kernel module signing, being able to reason
> > about the origin of a kernel module would be valuable in situations
> > where an OS already trusts a specific file system, file, etc, due to
> > things like security labels or an existing root of trust to a partition
> > through things like dm-verity.
> >
> > This introduces a new syscall (currently only 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.
> 
> Thanks.  Minor comments follow:

Rusty, sorry for bringing this up again, but with Kees' new syscall,
which passes in the file descriptor, appraising the integrity of kernel
modules could be like appraising the integrity of any other file on the
filesystem.  All that would be needed is a new security hook, which is
needed in anycase for IMA measurement.

The concerns with this approach, expressed in the past by David Howells,
are that it requires IMA-appraisal to be enabled, extended attribute
support, and changes to userspace tools.  Normally I wouldn't be too
concerned about filesystems that don't support extended attributes, but
the initramfs is currently cpio.  Perhaps this isn't an issue in
anycase, as the initramfs would be appraised in the secure boot
environment.

The first two concerns could be addressed by passing in a digital
signature, which the new syscall supports. The signature could be stored
as an extended attribute, appended to the kernel module or, as
originally described by Dmitry, in a .sig file. Where/how the signature
is stored would be left up to the distro's and userspace tool's
discretion.

When EVM/IMA-appraisal is enabled, it would either appraise the kernel
module based on the xattr, if available, or the supplied signature.
Otherwise, without EVM/IMA-appraisal enabled, the stub hook would
appraise the kernel module based on the supplied signature, calling
integrity_digsig_verify() directly.

This method is a consistent and extensible approach to verifying the
integrity of file data/metadata, including kernel modules. The only
downside to this approach, I think, is that it requires changes to the
userspace tool.

thanks,

Mimi

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

[Kees Cook]

On Fri, Sep 7, 2012 at 10:12 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Fri, 2012-09-07 at 09:45 +0930, Rusty Russell wrote:
>> Kees Cook <keescook@chromium.org> writes:
>> > Instead of (or in addition to) kernel module signing, being able to reason
>> > about the origin of a kernel module would be valuable in situations
>> > where an OS already trusts a specific file system, file, etc, due to
>> > things like security labels or an existing root of trust to a partition
>> > through things like dm-verity.
>> >
>> > This introduces a new syscall (currently only 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.
>>
>> Thanks.  Minor comments follow:
>
> Rusty, sorry for bringing this up again, but with Kees' new syscall,
> which passes in the file descriptor, appraising the integrity of kernel
> modules could be like appraising the integrity of any other file on the
> filesystem.  All that would be needed is a new security hook, which is
> needed in anycase for IMA measurement.

The second patch in this series provides such a hook.

> [...]
> This method is a consistent and extensible approach to verifying the
> integrity of file data/metadata, including kernel modules. The only
> downside to this approach, I think, is that it requires changes to the
> userspace tool.

I'm fine with this -- it's an expected change that I'll pursue with
glibc, kmod, etc. Without the userspace changes, nothing will use the
new syscall. :) I've already got kmod (and older module-init-tools)
patched to do this locally.

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Mimi Zohar]

On Fri, 2012-09-07 at 10:19 -0700, Kees Cook wrote:
> On Fri, Sep 7, 2012 at 10:12 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Fri, 2012-09-07 at 09:45 +0930, Rusty Russell wrote:
> >> Kees Cook <keescook@chromium.org> writes:
> >> > Instead of (or in addition to) kernel module signing, being able to reason
> >> > about the origin of a kernel module would be valuable in situations
> >> > where an OS already trusts a specific file system, file, etc, due to
> >> > things like security labels or an existing root of trust to a partition
> >> > through things like dm-verity.
> >> >
> >> > This introduces a new syscall (currently only 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.
> >>
> >> Thanks.  Minor comments follow:
> >
> > Rusty, sorry for bringing this up again, but with Kees' new syscall,
> > which passes in the file descriptor, appraising the integrity of kernel
> > modules could be like appraising the integrity of any other file on the
> > filesystem.  All that would be needed is a new security hook, which is
> > needed in anycase for IMA measurement.
> 
> The second patch in this series provides such a hook.

Thanks! Don't know how I missed it.

> 
> > [...]
> > This method is a consistent and extensible approach to verifying the
> > integrity of file data/metadata, including kernel modules. The only
> > downside to this approach, I think, is that it requires changes to the
> > userspace tool.
> 
> I'm fine with this -- it's an expected change that I'll pursue with
> glibc, kmod, etc. Without the userspace changes, nothing will use the
> new syscall. :) I've already got kmod (and older module-init-tools)
> patched to do this locally.

Great!

Mimi

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

[Rusty Russell]

Kees Cook <keescook@chromium.org> writes:
> On Fri, Sep 7, 2012 at 10:12 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> This method is a consistent and extensible approach to verifying the
>> integrity of file data/metadata, including kernel modules. The only
>> downside to this approach, I think, is that it requires changes to the
>> userspace tool.
>
> I'm fine with this -- it's an expected change that I'll pursue with
> glibc, kmod, etc. Without the userspace changes, nothing will use the
> new syscall. :) I've already got kmod (and older module-init-tools)
> patched to do this locally.

A syscall is the right way to do this.  But does it need to be done?

1) Do the LSM guys really want this hook?
2) Do we have a userspace which uses it?

If yes to both, and noone comes up with any creative complaints, I will
take the patch.

Cheers,
Rusty.

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

[Kees Cook]

On Sun, Sep 9, 2012 at 6:46 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>> On Fri, Sep 7, 2012 at 10:12 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>> This method is a consistent and extensible approach to verifying the
>>> integrity of file data/metadata, including kernel modules. The only
>>> downside to this approach, I think, is that it requires changes to the
>>> userspace tool.
>>
>> I'm fine with this -- it's an expected change that I'll pursue with
>> glibc, kmod, etc. Without the userspace changes, nothing will use the
>> new syscall. :) I've already got kmod (and older module-init-tools)
>> patched to do this locally.
>
> A syscall is the right way to do this.  But does it need to be done?
>
> 1) Do the LSM guys really want this hook?

The LSM hook half has already been acked by Serge and Eric, and I want
to use it in Yama as well.

> 2) Do we have a userspace which uses it?

Chrome OS will be using it; I have patches for kmod and
module-init-tools already.

> If yes to both, and noone comes up with any creative complaints, I will
> take the patch.

Sound good; thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

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

[James Morris]

On Mon, 10 Sep 2012, Rusty Russell wrote:

> Kees Cook <keescook@chromium.org> writes:
> > On Fri, Sep 7, 2012 at 10:12 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> This method is a consistent and extensible approach to verifying the
> >> integrity of file data/metadata, including kernel modules. The only
> >> downside to this approach, I think, is that it requires changes to the
> >> userspace tool.
> >
> > I'm fine with this -- it's an expected change that I'll pursue with
> > glibc, kmod, etc. Without the userspace changes, nothing will use the
> > new syscall. :) I've already got kmod (and older module-init-tools)
> > patched to do this locally.
> 
> A syscall is the right way to do this.  But does it need to be done?
> 
> 1) Do the LSM guys really want this hook?

Yes.

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

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

[H. Peter Anvin]

On 09/06/2012 11:13 AM, Kees Cook wrote:
> Instead of (or in addition to) kernel module signing, being able to reason
> about the origin of a kernel module would be valuable in situations
> where an OS already trusts a specific file system, file, etc, due to
> things like security labels or an existing root of trust to a partition
> through things like dm-verity.
>
> This introduces a new syscall (currently only 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.
>

Please use the standard naming convention, which is an f- prefix (i.e. 
finit_module()).

	-hpa

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

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

[Rusty Russell]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 09/06/2012 11:13 AM, Kees Cook wrote:
>> Instead of (or in addition to) kernel module signing, being able to reason
>> about the origin of a kernel module would be valuable in situations
>> where an OS already trusts a specific file system, file, etc, due to
>> things like security labels or an existing root of trust to a partition
>> through things like dm-verity.
>>
>> This introduces a new syscall (currently only 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.
>>
>
> Please use the standard naming convention, which is an f- prefix (i.e. 
> finit_module()).

Good point; I just did a replace here.

Thanks,
Rusty.

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

[Kees Cook]

On Wed, Sep 12, 2012 at 12:34 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
>
>> On 09/06/2012 11:13 AM, Kees Cook wrote:
>>> Instead of (or in addition to) kernel module signing, being able to reason
>>> about the origin of a kernel module would be valuable in situations
>>> where an OS already trusts a specific file system, file, etc, due to
>>> things like security labels or an existing root of trust to a partition
>>> through things like dm-verity.
>>>
>>> This introduces a new syscall (currently only 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.
>>>
>>
>> Please use the standard naming convention, which is an f- prefix (i.e.
>> finit_module()).
>
> Good point; I just did a replace here.

Ah, yes. Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Mimi Zohar]

On Wed, 2012-09-12 at 17:04 +0930, Rusty Russell wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
> > On 09/06/2012 11:13 AM, Kees Cook wrote:
> >> Instead of (or in addition to) kernel module signing, being able to reason
> >> about the origin of a kernel module would be valuable in situations
> >> where an OS already trusts a specific file system, file, etc, due to
> >> things like security labels or an existing root of trust to a partition
> >> through things like dm-verity.
> >>
> >> This introduces a new syscall (currently only 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.
> >>
> >
> > Please use the standard naming convention, which is an f- prefix (i.e. 
> > finit_module()).
> 
> Good point; I just did a replace here.

Have you pushed out the changes?  And if so, to where?

thanks,

Mimi

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

[Rusty Russell]

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

> On Wed, 2012-09-12 at 17:04 +0930, Rusty Russell wrote:
>> "H. Peter Anvin" <hpa@zytor.com> writes:
>> 
>> > On 09/06/2012 11:13 AM, Kees Cook wrote:
>> >> Instead of (or in addition to) kernel module signing, being able to reason
>> >> about the origin of a kernel module would be valuable in situations
>> >> where an OS already trusts a specific file system, file, etc, due to
>> >> things like security labels or an existing root of trust to a partition
>> >> through things like dm-verity.
>> >>
>> >> This introduces a new syscall (currently only 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.
>> >>
>> >
>> > Please use the standard naming convention, which is an f- prefix (i.e. 
>> > finit_module()).
>> 
>> Good point; I just did a replace here.
>
> Have you pushed out the changes?  And if so, to where?

No, I kept them in my patch series but out of linux-next, since I
thought you disliked the placement of the security hooks?

Cheers,
Rusty.

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

[Mimi Zohar]

On Wed, 2012-09-19 at 13:08 +0930, Rusty Russell wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> 
> > On Wed, 2012-09-12 at 17:04 +0930, Rusty Russell wrote:
> >> "H. Peter Anvin" <hpa@zytor.com> writes:
> >> 
> >> > On 09/06/2012 11:13 AM, Kees Cook wrote:
> >> >> Instead of (or in addition to) kernel module signing, being able to reason
> >> >> about the origin of a kernel module would be valuable in situations
> >> >> where an OS already trusts a specific file system, file, etc, due to
> >> >> things like security labels or an existing root of trust to a partition
> >> >> through things like dm-verity.
> >> >>
> >> >> This introduces a new syscall (currently only 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.
> >> >>
> >> >
> >> > Please use the standard naming convention, which is an f- prefix (i.e. 
> >> > finit_module()).
> >> 
> >> Good point; I just did a replace here.
> >
> > Have you pushed out the changes?  And if so, to where?
> 
> No, I kept them in my patch series but out of linux-next, since I
> thought you disliked the placement of the security hooks?

I thought about it some more.  The call to
security_kernel_module_from_file() from copy_module_from_user() doesn't
provide any information, not the buffer contents nor the signature.  The
only thing IMA-appraisal can do is to fail the request with
INTEGRITY_UNKNOWN.  This is reflected in the IMA-appraisal patch I
posted http://marc.info/?l=linux-security-module&m=134739023306344&w=2.

Please add my Acked-by: Mimi Zohar <zohar@us.ibm.com> 

thanks,

Mimi

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

[Kees Cook]

On Wed, Sep 19, 2012 at 7:41 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Wed, 2012-09-19 at 13:08 +0930, Rusty Russell wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>>
>> > On Wed, 2012-09-12 at 17:04 +0930, Rusty Russell wrote:
>> >> "H. Peter Anvin" <hpa@zytor.com> writes:
>> >>
>> >> > On 09/06/2012 11:13 AM, Kees Cook wrote:
>> >> >> Instead of (or in addition to) kernel module signing, being able to reason
>> >> >> about the origin of a kernel module would be valuable in situations
>> >> >> where an OS already trusts a specific file system, file, etc, due to
>> >> >> things like security labels or an existing root of trust to a partition
>> >> >> through things like dm-verity.
>> >> >>
>> >> >> This introduces a new syscall (currently only 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.
>> >> >>
>> >> >
>> >> > Please use the standard naming convention, which is an f- prefix (i.e.
>> >> > finit_module()).
>> >>
>> >> Good point; I just did a replace here.
>> >
>> > Have you pushed out the changes?  And if so, to where?
>>
>> No, I kept them in my patch series but out of linux-next, since I
>> thought you disliked the placement of the security hooks?
>
> I thought about it some more.  The call to
> security_kernel_module_from_file() from copy_module_from_user() doesn't
> provide any information, not the buffer contents nor the signature.  The
> only thing IMA-appraisal can do is to fail the request with
> INTEGRITY_UNKNOWN.  This is reflected in the IMA-appraisal patch I
> posted http://marc.info/?l=linux-security-module&m=134739023306344&w=2.
>
> Please add my Acked-by: Mimi Zohar <zohar@us.ibm.com>

FWIW, this was my intent: it is a way for the LSM to see an attempt to
load a module it can't reason about. If it wants to allow it blindly,
it can, otherwise is has the option to refuse it. I didn't want to
leave the old syscall unhooked.

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Kees Cook]

Instead of (or in addition to) kernel module signing, being able to reason
about the origin of a kernel module would be valuable in situations
where an OS already trusts a specific file system, file, etc, due to
things like security labels or an existing root of trust to a partition
through things like dm-verity.

This introduces a new syscall (currently only 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>
---
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                  |  214 ++++++++++++++++++++++++++-----------
 kernel/sys_ni.c                  |    1 +
 5 files changed, 154 insertions(+), 64 deletions(-)

diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 7a35a6e..98e76bf 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	init_module_fd		sys_init_module_fd
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index a582bfe..722476b 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	init_module_fd		sys_init_module_fd
 
 #
 # 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..6ee6e31 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_init_module_fd(int fd, const char __user *uargs);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 4edbd9c..c1e446e 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,102 @@ 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)
+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) {
-		err = -EFAULT;
+	err = copy_from_user(info->hdr, umod, info->len);
+	if (err)
 		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. */
+int copy_module_from_fd(int fd, struct load_info *info)
+{
+	struct file *file;
+	int err;
+	struct kstat stat;
+	unsigned long size;
+	off_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;
+
+	size = stat.size;
+	info->hdr = vmalloc(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 < size) {
+		bytes = kernel_read(file, pos, (char *)(info->hdr) + pos,
+				    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 +2916,17 @@ static int post_relocation(struct module *mod, const struct load_info *info)
 	return module_finalize(info->hdr, info->sechdrs, mod);
 }
 
+static int do_init_module(struct module *mod);
+
 /* 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 +2939,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 +2987,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 +3005,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 +3024,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,10 +3036,10 @@ 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. */
@@ -3007,21 +3054,10 @@ static void do_mod_ctors(struct module *mod)
 }
 
 /* This is where the real work happens */
-SYSCALL_DEFINE3(init_module, void __user *, umod,
-		unsigned long, len, const char __user *, uargs)
+static int do_init_module(struct module *mod)
 {
-	struct module *mod;
 	int ret = 0;
 
-	/* Must have permission */
-	if (!capable(CAP_SYS_MODULE) || modules_disabled)
-		return -EPERM;
-
-	/* Do all the hard work */
-	mod = load_module(umod, len, uargs);
-	if (IS_ERR(mod))
-		return PTR_ERR(mod);
-
 	blocking_notifier_call_chain(&module_notify_list,
 			MODULE_STATE_COMING, mod);
 
@@ -3091,6 +3127,56 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 	return 0;
 }
 
+static int init_module_permission(void)
+{
+	/* Must have permission */
+	if (!capable(CAP_SYS_MODULE) || modules_disabled)
+		return -EPERM;
+
+	return 0;
+}
+
+SYSCALL_DEFINE2(init_module_fd, int, fd, const char __user *, uargs)
+{
+	int err;
+	struct load_info info = { };
+
+	err = init_module_permission();
+	if (err)
+		return err;
+
+	pr_debug("init_module_fd: fd=%d, uargs=%p\n", fd, uargs);
+
+	if (fd < 0)
+		return -ENOEXEC;
+
+	err = copy_module_from_fd(fd, &info);
+	if (err)
+		return err;
+
+	return load_module(&info, uargs);
+}
+
+SYSCALL_DEFINE3(init_module, void __user *, umod,
+		unsigned long, len, const char __user *, uargs)
+{
+	int err;
+	struct load_info info = { };
+
+	err = init_module_permission();
+	if (err)
+		return err;
+
+	pr_debug("init_module: umod=%p, len=%lu, uargs=%p\n",
+	       umod, len, uargs);
+
+	err = copy_module_from_user(umod, len, &info);
+	if (err)
+		return err;
+
+	return load_module(&info, uargs);
+}
+
 static inline int within(unsigned long addr, void *start, unsigned long size)
 {
 	return ((void *)addr >= start && (void *)addr < start + size);
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index dbff751..3195f80 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_init_module_fd);
 cond_syscall(sys_delete_module);
 cond_syscall(sys_socketpair);
 cond_syscall(sys_bind);
-- 
1.7.0.4