On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zohar at 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 at linux.vnet.ibm.com>
I'm really liking this consolidation. :)
Acked-by: Kees Cook <keescook at 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
