Linux-Modules Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: linux-security-module <linux-security-module@vger.kernel.org>,
	"Luis R. Rodriguez" <mcgrof@suse.com>,
	Kexec Mailing List <kexec@lists.infradead.org>,
	linux-modules@vger.kernel.org,
	David Howells <dhowells@redhat.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	Eric Biederman <ebiederm@xmission.com>,
	Rusty Russell <rusty@rustcorp.com.au>
Subject: Re: [PATCH v3 16/22] module: replace copy_module_from_fd with kernel version
Date: Thu, 4 Feb 2016 10:04:05 -0800
Message-ID: <CAGXu5jJKbsDptQQ86Kx3bC8tWSLNYAHeqHCEU-4duq3gQ+z3kw@mail.gmail.com> (raw)
In-Reply-To: <1454526390-19792-17-git-send-email-zohar@linux.vnet.ibm.com>

On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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 <zohar@linux.vnet.ibm.com>

I'm really liking this consolidation. :)

Acked-by: Kees Cook <keescook@chromium.org>

-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

  reply index

Thread overview: 77+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-03 19:06 [PATCH v3 00/22] vfs: support for a common kernel file loader Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 01/22] ima: separate 'security.ima' reading functionality from collect Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 02/22] ima: refactor ima_policy_show() to display "ima_hooks" rules Mimi Zohar
2016-02-07 19:45   ` Petko Manolov
2016-02-10 19:33   ` Dmitry Kasatkin
2016-02-03 19:06 ` [PATCH v3 03/22] ima: use "ima_hooks" enum as function argument Mimi Zohar
2016-02-07 19:46   ` Petko Manolov
2016-02-10 19:35   ` Dmitry Kasatkin
2016-02-03 19:06 ` [PATCH v3 04/22] firmware: simplify dev_*() print messages for generic helpers Mimi Zohar
2016-02-04 17:26   ` Kees Cook
2016-02-03 19:06 ` [PATCH v3 05/22] firmware: move completing fw into a helper Mimi Zohar
2016-02-04 17:27   ` Kees Cook
2016-02-03 19:06 ` [PATCH v3 06/22] firmware: fold successful fw read early Mimi Zohar
2016-02-04 17:36   ` Kees Cook
2016-02-04 20:26     ` Luis R. Rodriguez
2016-02-03 19:06 ` [PATCH v3 07/22] vfs: define a generic function to read a file from the kernel Mimi Zohar
2016-02-04 17:41   ` Kees Cook
2016-02-03 19:06 ` [PATCH v3 08/22] vfs: define kernel_read_file_id enumeration Mimi Zohar
2016-02-04 17:41   ` Kees Cook
2016-02-04 19:45   ` Luis R. Rodriguez
2016-02-03 19:06 ` [PATCH v3 09/22] ima: provide buffer hash calculation function Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 10/22] ima: calculate the hash of a buffer using aynchronous hash(ahash) Mimi Zohar
2016-02-10 19:58   ` Dmitry Kasatkin
2016-02-03 19:06 ` [PATCH v3 11/22] ima: define a new hook to measure and appraise a file already in memory Mimi Zohar
2016-02-10 20:27   ` Dmitry Kasatkin
2016-02-03 19:06 ` [PATCH v3 12/22] vfs: define kernel_read_file_from_path Mimi Zohar
2016-02-04 17:46   ` Kees Cook
2016-02-04 19:47   ` Luis R. Rodriguez
2016-02-03 19:06 ` [PATCH v3 13/22] firmware: replace call to fw_read_file_contents() with kernel version Mimi Zohar
2016-02-04 17:56   ` Kees Cook
2016-02-04 19:51   ` Luis R. Rodriguez
2016-02-03 19:06 ` [PATCH v3 14/22] security: define kernel_read_file hook Mimi Zohar
2016-02-04 17:57   ` Kees Cook
2016-02-04 19:54   ` Luis R. Rodriguez
2016-02-11 16:54   ` Casey Schaufler
2016-02-11 19:35     ` Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 15/22] vfs: define kernel_copy_file_from_fd() Mimi Zohar
2016-02-04 17:58   ` Kees Cook
2016-02-04 19:55   ` Luis R. Rodriguez
2016-02-03 19:06 ` [PATCH v3 16/22] module: replace copy_module_from_fd with kernel version Mimi Zohar
2016-02-04 18:04   ` Kees Cook [this message]
2016-02-04 19:56   ` Luis R. Rodriguez
2016-02-05  0:19     ` Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 17/22] ima: remove firmware and module specific cached status info Mimi Zohar
2016-02-07 19:56   ` Petko Manolov
2016-02-10 20:18   ` Dmitry Kasatkin
2016-02-10 23:14     ` Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 18/22] kexec: replace call to copy_file_from_fd() with kernel version Mimi Zohar
2016-02-04 18:05   ` Kees Cook
2016-02-04 19:57   ` Luis R. Rodriguez
2016-02-12 12:50   ` Dave Young
2016-02-03 19:06 ` [PATCH v3 19/22] ima: support for kexec image and initramfs Mimi Zohar
2016-02-07 20:10   ` Petko Manolov
2016-02-08 23:34     ` Mimi Zohar
2016-02-10 21:09   ` Dmitry Kasatkin
2016-02-10 23:21     ` Mimi Zohar
     [not found]       ` <CACE9dm8OJ1cgbKszUG-pCiEMVarUFLLWi_jewVV-JEMGAJsA-g@mail.gmail.com>
2016-02-11  2:08         ` Mimi Zohar
2016-02-11  8:47           ` Dmitry Kasatkin
2016-02-11 12:16             ` Mimi Zohar
2016-02-12 12:53   ` Dave Young
2016-02-12 13:09     ` Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 20/22] ima: load policy using path Mimi Zohar
2016-02-07 19:59   ` Petko Manolov
2016-02-08  9:58     ` Dmitry Kasatkin
2016-02-08 10:35       ` Petko Manolov
2016-02-08 10:45         ` Dmitry Kasatkin
2016-02-08 21:12           ` Mimi Zohar
2016-02-09  7:47             ` Petko Manolov
2016-02-03 19:06 ` [PATCH v3 21/22] ima: measure and appraise the IMA policy itself Mimi Zohar
2016-02-07 20:01   ` Petko Manolov
2016-02-10 20:22   ` Dmitry Kasatkin
2016-02-10 23:15     ` Mimi Zohar
2016-02-03 19:06 ` [PATCH v3 22/22] ima: require signed IMA policy Mimi Zohar
2016-02-07 20:02   ` Petko Manolov
2016-02-10 20:24   ` Dmitry Kasatkin
2016-02-04 18:15 ` [PATCH v3 00/22] vfs: support for a common kernel file loader Kees Cook
2016-02-04 23:54   ` Mimi Zohar

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGXu5jJKbsDptQQ86Kx3bC8tWSLNYAHeqHCEU-4duq3gQ+z3kw@mail.gmail.com \
    --to=keescook@chromium.org \
    --cc=dhowells@redhat.com \
    --cc=dmitry.kasatkin@gmail.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=ebiederm@xmission.com \
    --cc=kexec@lists.infradead.org \
    --cc=linux-modules@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mcgrof@suse.com \
    --cc=rusty@rustcorp.com.au \
    --cc=zohar@linux.vnet.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Modules Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-modules/0 linux-modules/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-modules linux-modules/ https://lore.kernel.org/linux-modules \
		linux-modules@vger.kernel.org linux-modules@archiver.kernel.org
	public-inbox-index linux-modules


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-modules


AGPL code for this site: git clone https://public-inbox.org/ public-inbox