lkml.org 
[lkml]   [2012]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 1/2] module: add syscall to load module from fd
Date
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.


\
 
 \ /
  Last update: 2012-09-07 03:21    [W:0.364 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site