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

[PATCH 1/2] module: allow loading 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 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

[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>
---
 include/linux/security.h |   11 +++++++++++
 kernel/module.c          |    7 +++++++
 security/capability.c    |    6 ++++++
 security/security.c      |    5 +++++
 4 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 3dea6a9..634d09a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -693,6 +693,10 @@ 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 new kernel module from a file.
+ *	@file contains the file structure being loaded as a kernel module.
+ *	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 +1511,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 +1769,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 +2283,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 0be8c11..1fcc63f 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>
@@ -2447,6 +2448,12 @@ static Elf_Ehdr *copy_module_from_fd(unsigned int fd, unsigned long *len)
 	}
 	size = stat.size;
 
+	err = security_kernel_module_from_file(file);
+	if (err) {
+		hdr = ERR_PTR(err);
+		goto out;
+	}
+
 	hdr = vmalloc(size);
 	if (!hdr) {
 		hdr = ERR_PTR(-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: allow loading module from fd

[Serge Hallyn]

Quoting Kees Cook (keescook@chromium.org):
> 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>

Acked-by: Serge E. Hallyn <serge.hallyn@canonical.com>

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

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

[Serge Hallyn]

Quoting Kees Cook (keescook@chromium.org):
> 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 |   11 +++++++++++
>  kernel/module.c          |    7 +++++++
>  security/capability.c    |    6 ++++++
>  security/security.c      |    5 +++++
>  4 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3dea6a9..634d09a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -693,6 +693,10 @@ 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 new kernel module from a file.
> + *	@file contains the file structure being loaded as a kernel module.
> + *	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 +1511,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 +1769,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 +2283,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 0be8c11..1fcc63f 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>
> @@ -2447,6 +2448,12 @@ static Elf_Ehdr *copy_module_from_fd(unsigned int fd, unsigned long *len)
>  	}
>  	size = stat.size;
>  
> +	err = security_kernel_module_from_file(file);
> +	if (err) {
> +		hdr = ERR_PTR(err);
> +		goto out;
> +	}
> +
>  	hdr = vmalloc(size);
>  	if (!hdr) {
>  		hdr = ERR_PTR(-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: allow loading module from fd

[Rusty Russell]

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

Do you know why Linus hates ioctls?  It's because they encourage
casual introduction of new ABIs.

I see you managed the same feat with a different system call.  It avoids
the agony of debate a new system call would entail.  Very clever.

Now do it properly.
Rusty.

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

[Kees Cook]

On Thu, Sep 20, 2012 at 1:29 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri,  7 Sep 2012 11:38:13 -0700
> Kees Cook <keescook@chromium.org> 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.
>
> <scratches head>
>
> This is a really sketchy rationale and I would like to see a *lot* more
> about the reasoning behind this.  Who will use the feature?  How will
> they use it?  What value will they obtain from using it?  This
> description should be pitched at kernel literate non-security people
> who lack mind-reading powers, please.

I'm happy to expand this further. I tried to give some examples, but
I'll expand on those. The bottom line is that Chrome OS wants to load
modules only from its crypto-verified read-only root filesystem. Doing
external per-module signatures makes no sense for us, and we want to
be able to reason about the origin of a module, which is what this
syscall gets us.

> We'll need a manpage for this, and I'd suggest that preparing it sooner
> rather than later will help with the review of your proposal.

Sounds good. The manpage is see for the old init_module looks to be
wildly out of data anyway, so perhaps I can fix that too. I'll check
the manpage tree.

>> 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.
>>
>>
>> ...
>>
>> -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)
>
> can be made static, methinks.

Ah, yes, thanks. I'll fix the missing statics.

> `len' should have type size_t?

Yes, this was a left-over from the prior revision of this. I'll get
that fixed too.

>> +{
>> +     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;
>
> kstat.size had type loff_t.  Here it gets trucated to 32 bits on 32-bit
> machines.  Harmless I guess, but sloppy.

It's actually intentional, since I want to load the entire file into a
kmalloc-able region. I'll return EFBIG if we would truncate instead.

>> +     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) {
>
> `pos' should be loff_t as well.
>
>> +             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);
>
> I wonder if do_init_module() could have been moved to avoid the forward
> declaration.

I was hoping to make the patch more readable, but I can move things
around to avoid the pre-declaration.

>
>>  /* 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;
>>
>>
>> ...
>>
>> @@ -3091,6 +3127,56 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
>>       return 0;
>>  }
>>
>> +static int init_module_permission(void)
>
> "init_module_permission" -> initialises a module's permission.
>
> IOW, the name is poor.
>
>> +{
>> +     /* Must have permission */
>
> The world wouldn't end if this comment was omitted ;)

Sure, I'll fix this and the name.

>
>> +     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;
>
> hm, why?  Surely copy_module_from_fd()'s fget() will fail on a -ve fd?

Good point, I'll let it fall through.

>
>> +     err = copy_module_from_fd(fd, &info);
>> +     if (err)
>> +             return err;
>> +
>> +     return load_module(&info, uargs);
>> +}
>> +
>>
>> ...
>>
>

Thanks for the review! I'll get another version up shortly.

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Andrew Morton]

On Fri,  7 Sep 2012 11:38:13 -0700
Kees Cook <keescook@chromium.org> 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.

<scratches head>

This is a really sketchy rationale and I would like to see a *lot* more
about the reasoning behind this.  Who will use the feature?  How will
they use it?  What value will they obtain from using it?  This
description should be pitched at kernel literate non-security people
who lack mind-reading powers, please.

We'll need a manpage for this, and I'd suggest that preparing it sooner
rather than later will help with the review of your proposal.

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

can be made static, methinks.

`len' should have type size_t?

>  {
>  	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)

static

> +{
> +	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;

kstat.size had type loff_t.  Here it gets trucated to 32 bits on 32-bit
machines.  Harmless I guess, but sloppy.

> +	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) {

`pos' should be loff_t as well.

> +		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);

I wonder if do_init_module() could have been moved to avoid the forward
declaration.

>  /* 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;
>  
>
> ...
>
> @@ -3091,6 +3127,56 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
>  	return 0;
>  }
>  
> +static int init_module_permission(void)

"init_module_permission" -> initialises a module's permission.

IOW, the name is poor.

> +{
> +	/* Must have permission */

The world wouldn't end if this comment was omitted ;)

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

hm, why?  Surely copy_module_from_fd()'s fget() will fail on a -ve fd?

> +	err = copy_module_from_fd(fd, &info);
> +	if (err)
> +		return err;
> +
> +	return load_module(&info, uargs);
> +}
> +
>
> ...
>

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

[Mimi Zohar]

On Fri, 2012-09-07 at 11:38 -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.
> 
> 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 c1e446e..0ad03c4 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;
> +

For appraisal, having the security hook here is too early.  We need to
pass both the data and the signature.

>  	/* Suck in entire file: we'll want most of it. */
>  	info->hdr = vmalloc(info->len);
>  	if (!info->hdr)
> @@ -2468,6 +2473,10 @@ int copy_module_from_fd(int fd, struct load_info *info)
>  	if (err)
>  		goto out;
> 
> +	err = security_kernel_module_from_file(file);
> +	if (err)
> +		goto out;
> +

We probably want to pass the signature here as well, for those
filesystems that don't support extended attributes.

Mimi

>  	size = stat.size;
>  	info->hdr = vmalloc(size);
>  	if (!info->hdr) {
> 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 2/2] security: introduce kernel_module_from_file hook

[Eric Paris]

Acked-by: Eric Paris <eparis@redhat.com>

On Fri, Sep 7, 2012 at 2:38 PM, Kees Cook <keescook@chromium.org> wrote:
> 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 c1e446e..0ad03c4 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)
> @@ -2468,6 +2473,10 @@ int copy_module_from_fd(int fd, struct load_info *info)
>         if (err)
>                 goto out;
>
> +       err = security_kernel_module_from_file(file);
> +       if (err)
> +               goto out;
> +
>         size = stat.size;
>         info->hdr = vmalloc(size);
>         if (!info->hdr) {
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 c1e446e..0ad03c4 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)
@@ -2468,6 +2473,10 @@ int copy_module_from_fd(int fd, struct load_info *info)
 	if (err)
 		goto out;
 
+	err = security_kernel_module_from_file(file);
+	if (err)
+		goto out;
+
 	size = stat.size;
 	info->hdr = vmalloc(size);
 	if (!info->hdr) {
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 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