From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f172.google.com ([209.85.223.172]:36514 "EHLO mail-io0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754246AbcBDSEG (ORCPT ); Thu, 4 Feb 2016 13:04:06 -0500 Received: by mail-io0-f172.google.com with SMTP id g73so101850357ioe.3 for ; Thu, 04 Feb 2016 10:04:05 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1454526390-19792-17-git-send-email-zohar@linux.vnet.ibm.com> References: <1454526390-19792-1-git-send-email-zohar@linux.vnet.ibm.com> <1454526390-19792-17-git-send-email-zohar@linux.vnet.ibm.com> Date: Thu, 4 Feb 2016 10:04:05 -0800 Message-ID: Subject: Re: [PATCH v3 16/22] module: replace copy_module_from_fd with kernel version From: Kees Cook To: Mimi Zohar Cc: linux-security-module , "Luis R. Rodriguez" , Kexec Mailing List , linux-modules@vger.kernel.org, David Howells , David Woodhouse , Dmitry Torokhov , Dmitry Kasatkin , Eric Biederman , Rusty Russell Content-Type: text/plain; charset=UTF-8 Sender: owner-linux-modules@vger.kernel.org List-ID: On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar wrote: > Replace copy_module_from_fd() with kernel_read_file_from_fd(). > > Although none of the upstreamed LSMs define a kernel_module_from_file > hook, IMA is called, based on policy, to prevent unsigned kernel modules > from being loaded by the original kernel module syscall and to > measure/appraise signed kernel modules. > > The security function security_kernel_module_from_file() was called prior > to reading a kernel module. Preventing unsigned kernel modules from being > loaded by the original kernel module syscall remains on the pre-read > kernel_read_file() security hook. Instead of reading the kernel module > twice, once for measuring/appraising and again for loading the kernel > module, the signature validation is moved to the kernel_post_read_file() > security hook. > > This patch removes the security_kernel_module_from_file() hook and security > call. > > Signed-off-by: Mimi Zohar I'm really liking this consolidation. :) Acked-by: Kees Cook -Kees > --- > include/linux/fs.h | 1 + > include/linux/ima.h | 6 ---- > include/linux/lsm_hooks.h | 7 ---- > include/linux/security.h | 5 --- > kernel/module.c | 68 +++++---------------------------------- > security/integrity/ima/ima_main.c | 35 ++++++++------------ > security/security.c | 12 ------- > 7 files changed, 22 insertions(+), 112 deletions(-) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 5ba806b..9e1f1e3 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2528,6 +2528,7 @@ extern int do_pipe_flags(int *, int); > > enum kernel_read_file_id { > READING_FIRMWARE = 1, > + READING_MODULE, > READING_MAX_ID > }; > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 6adcaea..e6516cb 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -18,7 +18,6 @@ extern int ima_bprm_check(struct linux_binprm *bprm); > extern int ima_file_check(struct file *file, int mask, int opened); > extern void ima_file_free(struct file *file); > extern int ima_file_mmap(struct file *file, unsigned long prot); > -extern int ima_module_check(struct file *file); > extern int ima_read_file(struct file *file, enum kernel_read_file_id id); > extern int ima_post_read_file(struct file *file, void *buf, loff_t size, > enum kernel_read_file_id id); > @@ -44,11 +43,6 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot) > return 0; > } > > -static inline int ima_module_check(struct file *file) > -{ > - return 0; > -} > - > static inline int ima_read_file(struct file *file, enum kernel_read_file_id id) > { > return 0; > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index d32b7bd..cdee11c 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -546,12 +546,6 @@ > * 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. > * @kernel_read_file: > * Read a file specified by userspace. > * @file contains the file structure pointing to the file being read > @@ -1725,7 +1719,6 @@ struct security_hook_heads { > struct list_head kernel_read_file; > struct list_head kernel_post_read_file; > struct list_head kernel_module_request; > - struct list_head kernel_module_from_file; > struct list_head task_fix_setuid; > struct list_head task_setpgid; > struct list_head task_getpgid; > diff --git a/include/linux/security.h b/include/linux/security.h > index 071fb74..157f0cb 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -859,11 +859,6 @@ 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_kernel_read_file(struct file *file, > enum kernel_read_file_id id) > { > diff --git a/kernel/module.c b/kernel/module.c > index 8f051a1..d77c4f1 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -2665,7 +2665,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len, > if (info->len < sizeof(*(info->hdr))) > return -ENOEXEC; > > - err = security_kernel_module_from_file(NULL); > + err = security_kernel_read_file(NULL, READING_MODULE); > if (err) > return err; > > @@ -2683,63 +2683,6 @@ static int copy_module_from_user(const void __user *umod, unsigned long len, > return 0; > } > > -/* Sets info->hdr and info->len. */ > -static int copy_module_from_fd(int fd, struct load_info *info) > -{ > - struct fd f = fdget(fd); > - int err; > - struct kstat stat; > - loff_t pos; > - ssize_t bytes = 0; > - > - if (!f.file) > - return -ENOEXEC; > - > - err = security_kernel_module_from_file(f.file); > - if (err) > - goto out; > - > - err = vfs_getattr(&f.file->f_path, &stat); > - if (err) > - goto out; > - > - if (stat.size > INT_MAX) { > - err = -EFBIG; > - goto out; > - } > - > - /* Don't hand 0 to vmalloc, it whines. */ > - if (stat.size == 0) { > - err = -EINVAL; > - goto out; > - } > - > - info->hdr = vmalloc(stat.size); > - if (!info->hdr) { > - err = -ENOMEM; > - goto out; > - } > - > - pos = 0; > - while (pos < stat.size) { > - bytes = kernel_read(f.file, pos, (char *)(info->hdr) + pos, > - stat.size - pos); > - if (bytes < 0) { > - vfree(info->hdr); > - err = bytes; > - goto out; > - } > - if (bytes == 0) > - break; > - pos += bytes; > - } > - info->len = pos; > - > -out: > - fdput(f); > - return err; > -} > - > static void free_copy(struct load_info *info) > { > vfree(info->hdr); > @@ -3602,8 +3545,10 @@ SYSCALL_DEFINE3(init_module, void __user *, umod, > > SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) > { > - int err; > struct load_info info = { }; > + loff_t size; > + void *hdr; > + int err; > > err = may_init_module(); > if (err) > @@ -3615,9 +3560,12 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags) > |MODULE_INIT_IGNORE_VERMAGIC)) > return -EINVAL; > > - err = copy_module_from_fd(fd, &info); > + err = kernel_read_file_from_fd(fd, &hdr, &size, INT_MAX, > + READING_MODULE); > if (err) > return err; > + info.hdr = hdr; > + info.len = size; > > return load_module(&info, uargs, flags); > } > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 6f79bdf..1e91d94 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -316,28 +316,6 @@ int ima_file_check(struct file *file, int mask, int opened) > EXPORT_SYMBOL_GPL(ima_file_check); > > /** > - * ima_module_check - based on policy, collect/store/appraise measurement. > - * @file: pointer to the file to be measured/appraised > - * > - * Measure/appraise kernel modules based on policy. > - * > - * On success return 0. On integrity appraisal error, assuming the file > - * is in policy and IMA-appraisal is in enforcing mode, return -EACCES. > - */ > -int ima_module_check(struct file *file) > -{ > - if (!file) { > -#ifndef CONFIG_MODULE_SIG_FORCE > - if ((ima_appraise & IMA_APPRAISE_MODULES) && > - (ima_appraise & IMA_APPRAISE_ENFORCE)) > - return -EACCES; /* INTEGRITY_UNKNOWN */ > -#endif > - return 0; /* We rely on module signature checking */ > - } > - return process_measurement(file, NULL, 0, MAY_EXEC, MODULE_CHECK, 0); > -} > - > -/** > * ima_read_file - pre-measure/appraise hook decision based on policy > * @file: pointer to the file to be measured/appraised/audit > * @read_id: caller identifier > @@ -350,6 +328,14 @@ int ima_module_check(struct file *file) > */ > int ima_read_file(struct file *file, enum kernel_read_file_id read_id) > { > + if (!file && read_id == READING_MODULE) { > +#ifndef CONFIG_MODULE_SIG_FORCE > + if ((ima_appraise & IMA_APPRAISE_MODULES) && > + (ima_appraise & IMA_APPRAISE_ENFORCE)) > + return -EACCES; /* INTEGRITY_UNKNOWN */ > +#endif > + return 0; /* We rely on module signature checking */ > + } > return 0; > } > > @@ -378,6 +364,9 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, > return 0; > } > > + if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */ > + return 0; > + > if (!file && (!buf || size == 0)) { /* should never happen */ > if (ima_appraise & IMA_APPRAISE_ENFORCE) > return -EACCES; > @@ -386,6 +375,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size, > > if (read_id == READING_FIRMWARE) > func = FIRMWARE_CHECK; > + else if (read_id == READING_MODULE) > + func = MODULE_CHECK; > > return process_measurement(file, buf, size, MAY_READ, func, 0); > } > diff --git a/security/security.c b/security/security.c > index 1728fe2..3573c82 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -889,16 +889,6 @@ int security_kernel_module_request(char *kmod_name) > return call_int_hook(kernel_module_request, 0, kmod_name); > } > > -int security_kernel_module_from_file(struct file *file) > -{ > - int ret; > - > - ret = call_int_hook(kernel_module_from_file, 0, file); > - if (ret) > - return ret; > - return ima_module_check(file); > -} > - > int security_kernel_read_file(struct file *file, enum kernel_read_file_id id) > { > int ret; > @@ -1703,8 +1693,6 @@ struct security_hook_heads security_hook_heads = { > LIST_HEAD_INIT(security_hook_heads.kernel_create_files_as), > .kernel_module_request = > LIST_HEAD_INIT(security_hook_heads.kernel_module_request), > - .kernel_module_from_file = > - LIST_HEAD_INIT(security_hook_heads.kernel_module_from_file), > .kernel_read_file = > LIST_HEAD_INIT(security_hook_heads.kernel_read_file), > .kernel_post_read_file = > -- > 2.1.0 > -- Kees Cook Chrome OS & Brillo Security