On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> The circular lockdep is caused by allocating the 'iint' for mmapped
> files. Originally when an 'iint' was allocated for every inode
> in inode_alloc_security(), before the inode was accessible, no
> locking was necessary. Commits bc7d2a3e and 196f518 changed this
> behavior and allocated the 'iint' on a per need basis, resulting in
> the mmap_sem being taken before the i_mutex for mmapped files.
>
> Possible unsafe locking scenario:
> CPU0 CPU1
> ---- ----
> lock(&mm->mmap_sem);
> lock(&sb->s_type->i_mutex_key);
> lock(&mm->mmap_sem);
> lock(&sb->s_type->i_mutex_key);
>
> This patch adds a new hook ima_file_premmap() to pre-allocate the
> iint, preventing the i_mutex being taken after the mmap_sem, and
> defines a do_mmap() helper function do_mmap_with_sem().
>
> Before making this sort of change throughout, perhaps someone sees
> a better option?
>
Hi,
After a bit of thinking I remembered that I have seen ima hooks are
called for the same file...
i have done call tracing again and found out that.
FILE_CHECK is ALWAYS called before FILE_MMAP or BPRM_CHECK.
So when 2 above are called, file is already verified..
Indeed, in both cases before mmap or exec, the file is opened with
do_filp_open().
Are these completely useless then?
FILE_MMAP or BPRM_CHECK
- Dmitry
> thanks,
>
> Mimi
> ---
> arch/x86/ia32/ia32_aout.c | 24 ++++++++++--------------
> fs/binfmt_aout.c | 18 ++++++------------
> fs/binfmt_elf.c | 9 +++++----
> fs/binfmt_elf_fdpic.c | 7 +++----
> fs/binfmt_flat.c | 5 ++---
> fs/binfmt_som.c | 21 +++++++++------------
> include/linux/ima.h | 6 ++++++
> include/linux/mm.h | 5 +++++
> ipc/shm.c | 4 +++-
> mm/mmap.c | 22 ++++++++++++++++++++++
> mm/nommu.c | 5 +++++
> security/integrity/ima/ima.h | 2 +-
> security/integrity/ima/ima_main.c | 27 +++++++++++++++++++++++++++
> security/integrity/ima/ima_policy.c | 2 ++
> 14 files changed, 106 insertions(+), 51 deletions(-)
>
> diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
> index fd84387..39fb63d 100644
> --- a/arch/x86/ia32/ia32_aout.c
> +++ b/arch/x86/ia32/ia32_aout.c
> @@ -380,26 +380,22 @@ static int load_aout_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> goto beyond_if;
> }
>
> - down_write(¤t->mm->mmap_sem);
> - error = do_mmap(bprm->file, N_TXTADDR(ex), ex.a_text,
> + error = do_mmap_with_sem(bprm->file, N_TXTADDR(ex), ex.a_text,
> PROT_READ | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE |
> MAP_EXECUTABLE | MAP_32BIT,
> - fd_offset);
> - up_write(¤t->mm->mmap_sem);
> + fd_offset, ¤t->mm->mmap_sem);
>
> if (error != N_TXTADDR(ex)) {
> send_sig(SIGKILL, current, 0);
> return error;
> }
>
> - down_write(¤t->mm->mmap_sem);
> - error = do_mmap(bprm->file, N_DATADDR(ex), ex.a_data,
> + error = do_mmap_with_sem(bprm->file, N_DATADDR(ex), ex.a_data,
> PROT_READ | PROT_WRITE | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE |
> MAP_EXECUTABLE | MAP_32BIT,
> - fd_offset + ex.a_text);
> - up_write(¤t->mm->mmap_sem);
> + fd_offset + ex.a_text, ¤t->mm->mmap_sem);
> if (error != N_DATADDR(ex)) {
> send_sig(SIGKILL, current, 0);
> return error;
> @@ -491,13 +487,13 @@ static int load_aout_library(struct file *file)
> retval = 0;
> goto out;
> }
> +
> /* Now use mmap to map the library into memory. */
> - down_write(¤t->mm->mmap_sem);
> - error = do_mmap(file, start_addr, ex.a_text + ex.a_data,
> - PROT_READ | PROT_WRITE | PROT_EXEC,
> - MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_32BIT,
> - N_TXTOFF(ex));
> - up_write(¤t->mm->mmap_sem);
> + error = do_mmap_with_sem(file, start_addr, ex.a_text + ex.a_data,
> + PROT_READ | PROT_WRITE | PROT_EXEC,
> + MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE
> + | MAP_32BIT, N_TXTOFF(ex),
> + ¤t->mm->mmap_sem);
> retval = error;
> if (error != start_addr)
> goto out;
> diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
> index a6395bd..d8a4317 100644
> --- a/fs/binfmt_aout.c
> +++ b/fs/binfmt_aout.c
> @@ -320,24 +320,20 @@ static int load_aout_binary(struct linux_binprm * bprm, struct pt_regs * regs)
> goto beyond_if;
> }
>
> - down_write(¤t->mm->mmap_sem);
> - error = do_mmap(bprm->file, N_TXTADDR(ex), ex.a_text,
> + error = do_mmap_with_sem(bprm->file, N_TXTADDR(ex), ex.a_text,
> PROT_READ | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
> - fd_offset);
> - up_write(¤t->mm->mmap_sem);
> + fd_offset, ¤t->mm->mmap_sem);
>
> if (error != N_TXTADDR(ex)) {
> send_sig(SIGKILL, current, 0);
> return error;
> }
>
> - down_write(¤t->mm->mmap_sem);
> - error = do_mmap(bprm->file, N_DATADDR(ex), ex.a_data,
> + error = do_mmap_with_sem(bprm->file, N_DATADDR(ex), ex.a_data,
> PROT_READ | PROT_WRITE | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
> - fd_offset + ex.a_text);
> - up_write(¤t->mm->mmap_sem);
> + fd_offset + ex.a_text, ¤t->mm->mmap_sem);
> if (error != N_DATADDR(ex)) {
> send_sig(SIGKILL, current, 0);
> return error;
> @@ -427,12 +423,10 @@ static int load_aout_library(struct file *file)
> goto out;
> }
> /* Now use mmap to map the library into memory. */
> - down_write(¤t->mm->mmap_sem);
> - error = do_mmap(file, start_addr, ex.a_text + ex.a_data,
> + error = do_mmap_with_sem(file, start_addr, ex.a_text + ex.a_data,
> PROT_READ | PROT_WRITE | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE,
> - N_TXTOFF(ex));
> - up_write(¤t->mm->mmap_sem);
> + N_TXTOFF(ex), ¤t->mm->mmap_sem);
> retval = error;
> if (error != start_addr)
> goto out;
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 21ac5ee..9946b33 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -28,6 +28,7 @@
> #include <linux/highmem.h>
> #include <linux/pagemap.h>
> #include <linux/security.h>
> +#include <linux/ima.h>
> #include <linux/random.h>
> #include <linux/elf.h>
> #include <linux/utsname.h>
> @@ -330,6 +331,7 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> if (!size)
> return addr;
>
> + ima_file_premmap(filep, prot);
> down_write(¤t->mm->mmap_sem);
> /*
> * total_size is the size of the ELF (interpreter) image.
> @@ -1051,16 +1053,15 @@ static int load_elf_library(struct file *file)
> eppnt++;
>
> /* Now use mmap to map the library into memory. */
> - down_write(¤t->mm->mmap_sem);
> - error = do_mmap(file,
> + error = do_mmap_with_sem(file,
> ELF_PAGESTART(eppnt->p_vaddr),
> (eppnt->p_filesz +
> ELF_PAGEOFFSET(eppnt->p_vaddr)),
> PROT_READ | PROT_WRITE | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE,
> (eppnt->p_offset -
> - ELF_PAGEOFFSET(eppnt->p_vaddr)));
> - up_write(¤t->mm->mmap_sem);
> + ELF_PAGEOFFSET(eppnt->p_vaddr)),
> + ¤t->mm->mmap_sem);
> if (error != ELF_PAGESTART(eppnt->p_vaddr))
> goto out_free_ph;
>
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index 30745f4..b5baddf 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -1097,10 +1097,9 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>
> /* create the mapping */
> disp = phdr->p_vaddr & ~PAGE_MASK;
> - down_write(&mm->mmap_sem);
> - maddr = do_mmap(file, maddr, phdr->p_memsz + disp, prot, flags,
> - phdr->p_offset - disp);
> - up_write(&mm->mmap_sem);
> + maddr = do_mmap_with_sem(file, maddr, phdr->p_memsz + disp,
> + prot, flags, phdr->p_offset - disp),
> + &mm->mmap_sem);
>
> kdebug("mmap[%d] <file> sz=%lx pr=%x fl=%x of=%lx --> %08lx",
> loop, phdr->p_memsz + disp, prot, flags,
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 1bffbe0..f617ef9 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -543,10 +543,9 @@ static int load_flat_file(struct linux_binprm * bprm,
> */
> DBG_FLT("BINFMT_FLAT: ROM mapping of file (we hope)\n");
>
> - down_write(¤t->mm->mmap_sem);
> textpos = do_mmap(bprm->file, 0, text_len, PROT_READ|PROT_EXEC,
> - MAP_PRIVATE|MAP_EXECUTABLE, 0);
> - up_write(¤t->mm->mmap_sem);
> + MAP_PRIVATE|MAP_EXECUTABLE, 0,
> + ¤t->mm->mmap_sem);
> if (!textpos || IS_ERR_VALUE(textpos)) {
> if (!textpos)
> textpos = (unsigned long) -ENOMEM;
> diff --git a/fs/binfmt_som.c b/fs/binfmt_som.c
> index cc8560f..0ff9b98 100644
> --- a/fs/binfmt_som.c
> +++ b/fs/binfmt_som.c
> @@ -147,10 +147,9 @@ static int map_som_binary(struct file *file,
> code_size = SOM_PAGEALIGN(hpuxhdr->exec_tsize);
> current->mm->start_code = code_start;
> current->mm->end_code = code_start + code_size;
> - down_write(¤t->mm->mmap_sem);
> - retval = do_mmap(file, code_start, code_size, prot,
> - flags, SOM_PAGESTART(hpuxhdr->exec_tfile));
> - up_write(¤t->mm->mmap_sem);
> + retval = do_mmap_with_sem(file, code_start, code_size, prot,
> + flags, SOM_PAGESTART(hpuxhdr->exec_tfile),
> + current->mm->mmap_sem);
> if (retval < 0 && retval > -1024)
> goto out;
>
> @@ -158,20 +157,18 @@ static int map_som_binary(struct file *file,
> data_size = SOM_PAGEALIGN(hpuxhdr->exec_dsize);
> current->mm->start_data = data_start;
> current->mm->end_data = bss_start = data_start + data_size;
> - down_write(¤t->mm->mmap_sem);
> - retval = do_mmap(file, data_start, data_size,
> + retval = do_mmap_with_sem(file, data_start, data_size,
> prot | PROT_WRITE, flags,
> - SOM_PAGESTART(hpuxhdr->exec_dfile));
> - up_write(¤t->mm->mmap_sem);
> + SOM_PAGESTART(hpuxhdr->exec_dfile),
> + current->mm->mmap_sem);
> if (retval < 0 && retval > -1024)
> goto out;
>
> som_brk = bss_start + SOM_PAGEALIGN(hpuxhdr->exec_bsize);
> current->mm->start_brk = current->mm->brk = som_brk;
> - down_write(¤t->mm->mmap_sem);
> - retval = do_mmap(NULL, bss_start, som_brk - bss_start,
> - prot | PROT_WRITE, MAP_FIXED | MAP_PRIVATE, 0);
> - up_write(¤t->mm->mmap_sem);
> + retval = do_mmap_with_sem(NULL, bss_start, som_brk - bss_start,
> + prot | PROT_WRITE, MAP_FIXED | MAP_PRIVATE, 0,
> + ¤t->mm->mmap_sem);
> if (retval > 0 || retval < -1024)
> retval = 0;
> out:
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 6ac8e50..3862639 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -17,6 +17,7 @@ struct linux_binprm;
> extern int ima_bprm_check(struct linux_binprm *bprm);
> extern int ima_file_check(struct file *file, int mask);
> extern void ima_file_free(struct file *file);
> +extern int ima_file_premmap(struct file *file, unsigned long prot);
> extern int ima_file_mmap(struct file *file, unsigned long prot);
>
> #else
> @@ -35,6 +36,11 @@ static inline void ima_file_free(struct file *file)
> return;
> }
>
> +static inline int ima_file_premmap(struct file *file, unsigned long prot)
> +{
> + return 0;
> +}
> +
> static inline int ima_file_mmap(struct file *file, unsigned long prot)
> {
> return 0;
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 3dc3a8c..bf8da47 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1417,6 +1417,11 @@ out:
> return ret;
> }
>
> +extern unsigned long do_mmap_with_sem(struct file *file, unsigned long addr,
> + unsigned long len, unsigned long prot,
> + unsigned long flag, unsigned long offset,
> + struct rw_semaphore *mmap_sem);
> +
> extern int do_munmap(struct mm_struct *, unsigned long, size_t);
>
> extern unsigned long do_brk(unsigned long, unsigned long);
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 02ecf2c..e434c37 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -30,6 +30,7 @@
> #include <linux/mman.h>
> #include <linux/shmem_fs.h>
> #include <linux/security.h>
> +#include <linux/ima.h>
> #include <linux/syscalls.h>
> #include <linux/audit.h>
> #include <linux/capability.h>
> @@ -1042,7 +1043,8 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr)
> addr > current->mm->start_stack - size - PAGE_SIZE * 5)
> goto invalid;
> }
> -
> +
> + ima_file_premmap(file, prot);
> user_addr = do_mmap (file, addr, size, prot, flags, 0);
> *raddr = user_addr;
> err = 0;
> diff --git a/mm/mmap.c b/mm/mmap.c
> index eae90af..71ed785 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -20,6 +20,7 @@
> #include <linux/fs.h>
> #include <linux/personality.h>
> #include <linux/security.h>
> +#include <linux/ima.h>
> #include <linux/hugetlb.h>
> #include <linux/profile.h>
> #include <linux/export.h>
> @@ -1107,17 +1108,38 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
> }
>
> flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
> + retval = ima_file_premmap(file, prot);
> + if (retval)
> + goto err_out;
>
> down_write(¤t->mm->mmap_sem);
> retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
> up_write(¤t->mm->mmap_sem);
>
> +err_out:
> if (file)
> fput(file);
> out:
> return retval;
> }
>
> +unsigned long do_mmap_with_sem(struct file *file,
> + unsigned long addr,
> + unsigned long len, unsigned long prot,
> + unsigned long flag, unsigned long offset,
> + struct rw_semaphore *mmap_sem)
> +{
> + unsigned long ret;
> +
> + ima_file_premmap(file, prot);
> +
> + down_write(mmap_sem);
> + ret = do_mmap(file, addr, len, prot, flag, offset);
> + up_write(mmap_sem);
> + return ret;
> +}
> +EXPORT_SYMBOL(do_mmap_with_sem);
> +
> #ifdef __ARCH_WANT_SYS_OLD_MMAP
> struct mmap_arg_struct {
> unsigned long addr;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 73419c5..bcb5b5e 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -27,6 +27,7 @@
> #include <linux/mount.h>
> #include <linux/personality.h>
> #include <linux/security.h>
> +#include <linux/ima.h>
> #include <linux/syscalls.h>
> #include <linux/audit.h>
>
> @@ -1485,11 +1486,15 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
> }
>
> flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
> + retval = ima_file_premmap(file, prot);
> + if (retval)
> + goto err_out;
>
> down_write(¤t->mm->mmap_sem);
> retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
> up_write(¤t->mm->mmap_sem);
>
> +err_out:
> if (file)
> fput(file);
> out:
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 3ccf7ac..80819aa 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -114,7 +114,7 @@ struct integrity_iint_cache *integrity_iint_insert(struct inode *inode);
> struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>
> /* IMA policy related functions */
> -enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
> +enum ima_hooks { FILE_CHECK = 1, FILE_PREMMAP, FILE_MMAP, BPRM_CHECK };
>
> int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask);
> void ima_init_policy(void);
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 1eff5cb..1df7ede 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -140,6 +140,9 @@ retry:
> return rc;
> }
>
> + if (function == FILE_PREMMAP) /* defer to FILE_MMAP */
> + return 0;
> +
> mutex_lock(&iint->mutex);
>
> rc = iint->flags & IMA_MEASURED ? 1 : 0;
> @@ -153,6 +156,30 @@ out:
> mutex_unlock(&iint->mutex);
> return rc;
> }
> +
> +/**
> + * ima_file_premmap - based on policy allocate the 'iint'
> + * @file: pointer to the file to be measured (May be NULL)
> + * @prot: contains the protection that will be applied by the kernel.
> + *
> + * Based on the measurement policy, pre-allocate the iint before the
> + * mmap_sem is taken, but defer the actual measurement until
> + * security_file_mmap().
> + *
> + * (Pre-allocating the iint, prevents the i_mutex being taken after the
> + * mmap_sem.)
> + */
> +int ima_file_premmap(struct file *file, unsigned long prot)
> +{
> + int rc;
> +
> + if (!file)
> + return 0;
> + if (prot & PROT_EXEC)
> + rc = process_measurement(file, file->f_dentry->d_name.name,
> + MAY_EXEC, FILE_PREMMAP);
> + return 0;
> +}
>
> /**
> * ima_file_mmap - based on policy, collect/store measurement.
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index d661afb..3a0e042 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -64,6 +64,8 @@ static struct ima_measure_rule_entry default_rules[] = {
> {.action = DONT_MEASURE,.fsmagic = TMPFS_MAGIC,.flags = IMA_FSMAGIC},
> {.action = DONT_MEASURE,.fsmagic = SECURITYFS_MAGIC,.flags = IMA_FSMAGIC},
> {.action = DONT_MEASURE,.fsmagic = SELINUX_MAGIC,.flags = IMA_FSMAGIC},
> + {.action = MEASURE,.func = FILE_PREMMAP,.mask = MAY_EXEC,
> + .flags = IMA_FUNC | IMA_MASK},
> {.action = MEASURE,.func = FILE_MMAP,.mask = MAY_EXEC,
> .flags = IMA_FUNC | IMA_MASK},
> {.action = MEASURE,.func = BPRM_CHECK,.mask = MAY_EXEC,
> --
> 1.7.6.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
