[PATCH 1/2] module: allow loading module from fd

* [PATCH 1/2] module: allow loading module from fd
@ 2012-08-29 21:29 Kees Cook
  2012-08-29 21:29 ` [PATCH 2/2] security: introduce kernel_module_from_file hook Kees Cook
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Kees Cook @ 2012-08-29 21:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Rusty Russell, Serge Hallyn, James Morris, Al Viro, Eric Paris,
	Kees Cook, Jiri Kosina, linux-security-module

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 changes the init_module syscall so that when the first argument
(blob address) is NULL, the second argument is used as a file descriptor
to the module (instead of length). The third argument (module arguments)
remains unchanged.

Some alternatives to overloading the existing syscall are:
 - write a new syscall (seemed unnecessary)
 - add an fd ioctl (awful)
 - enhance the ELF binfmt loader (complex)

It seemed most sensible to avoid introducing new or crazy interfaces
or further complicating the ELF loader. Instead, just use the existing
syscall in a new way. Tools using the fd argument style can trivially
downgrade to the blob argument style when they see an EFAULT error.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/module.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 87 insertions(+), 10 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 4edbd9c..0be8c11 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,23 +2400,99 @@ static inline void kmemleak_load_module(const struct module *mod,
 }
 #endif
 
-/* 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 Elf_Ehdr *copy_module_from_user(const void __user *umod,
+				       unsigned long len)
 {
-	int err;
 	Elf_Ehdr *hdr;
 
 	if (len < sizeof(*hdr))
-		return -ENOEXEC;
+		return ERR_PTR(-ENOEXEC);
 
 	/* Suck in entire file: we'll want most of it. */
-	if ((hdr = vmalloc(len)) == NULL)
-		return -ENOMEM;
+	hdr = vmalloc(len);
+	if (!hdr)
+		return ERR_PTR(-ENOMEM);
 
 	if (copy_from_user(hdr, umod, len) != 0) {
-		err = -EFAULT;
+		vfree(hdr);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return hdr;
+}
+
+static Elf_Ehdr *copy_module_from_fd(unsigned int fd, unsigned long *len)
+{
+	struct file *file;
+	int err;
+	Elf_Ehdr *hdr;
+	struct kstat stat;
+	unsigned long size;
+	off_t pos;
+	ssize_t bytes = 0;
+
+	file = fget(fd);
+	if (!file)
+		return ERR_PTR(-ENOEXEC);
+
+	err = vfs_getattr(file->f_vfsmnt, file->f_dentry, &stat);
+	if (err) {
+		hdr = ERR_PTR(err);
+		goto out;
+	}
+
+	if (stat.size > INT_MAX) {
+		hdr = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+	size = stat.size;
+
+	hdr = vmalloc(size);
+	if (!hdr) {
+		hdr = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+
+	pos = 0;
+	while (pos < size) {
+		bytes = kernel_read(file, pos, (char *)hdr + pos, size - pos);
+		if (bytes < 0) {
+			vfree(hdr);
+			hdr = ERR_PTR(bytes);
+			goto out;
+		}
+		if (bytes == 0)
+			break;
+		pos += bytes;
+	}
+	*len = pos;
+
+out:
+	fput(file);
+	return hdr;
+}
+
+/* Sets info->hdr and info->len. */
+static int copy_and_check(struct load_info *info,
+			  const void __user *umod, unsigned long len)
+{
+	int err;
+	Elf_Ehdr *hdr;
+
+	if (umod == NULL) {
+		unsigned int fd;
+
+		if (len < 0 || len > INT_MAX)
+			return -ENOEXEC;
+		fd = len;
+
+		hdr = copy_module_from_fd(fd, &len);
+	} else
+		hdr = copy_module_from_user(umod, len);
+	if (IS_ERR(hdr))
+		return PTR_ERR(hdr);
+	if (len < sizeof(*hdr)) {
+		err = -ENOEXEC;
 		goto free_hdr;
 	}
 
@@ -2875,7 +2952,7 @@ static struct module *load_module(void __user *umod,
 	       umod, len, uargs);
 
 	/* Copy in the blobs from userspace, check they are vaguely sane. */
-	err = copy_and_check(&info, umod, len, uargs);
+	err = copy_and_check(&info, umod, len);
 	if (err)
 		return ERR_PTR(err);
 
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread