[RFC][PATCH] ima: fix lockdep circular locking dependency

[RFC][PATCH] ima: fix lockdep circular locking dependency

[Mimi Zohar]

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?

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(&current->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(&current->mm->mmap_sem);
+				fd_offset, &current->mm->mmap_sem);
 
 		if (error != N_TXTADDR(ex)) {
 			send_sig(SIGKILL, current, 0);
 			return error;
 		}
 
-		down_write(&current->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(&current->mm->mmap_sem);
+				fd_offset + ex.a_text, &current->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(&current->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(&current->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),
+				 &current->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(&current->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(&current->mm->mmap_sem);
+			fd_offset, &current->mm->mmap_sem);
 
 		if (error != N_TXTADDR(ex)) {
 			send_sig(SIGKILL, current, 0);
 			return error;
 		}
 
-		down_write(&current->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(&current->mm->mmap_sem);
+				fd_offset + ex.a_text, &current->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(&current->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(&current->mm->mmap_sem);
+			N_TXTOFF(ex), &current->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(&current->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(&current->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(&current->mm->mmap_sem);
+			 ELF_PAGEOFFSET(eppnt->p_vaddr)),
+			&current->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(&current->mm->mmap_sem);
 		textpos = do_mmap(bprm->file, 0, text_len, PROT_READ|PROT_EXEC,
-				  MAP_PRIVATE|MAP_EXECUTABLE, 0);
-		up_write(&current->mm->mmap_sem);
+				  MAP_PRIVATE|MAP_EXECUTABLE, 0,
+				  &current->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(&current->mm->mmap_sem);
-	retval = do_mmap(file, code_start, code_size, prot,
-			flags, SOM_PAGESTART(hpuxhdr->exec_tfile));
-	up_write(&current->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(&current->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(&current->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(&current->mm->mmap_sem);
-	retval = do_mmap(NULL, bss_start, som_brk - bss_start,
-			prot | PROT_WRITE, MAP_FIXED | MAP_PRIVATE, 0);
-	up_write(&current->mm->mmap_sem);
+	retval = do_mmap_with_sem(NULL, bss_start, som_brk - bss_start,
+			prot | PROT_WRITE, MAP_FIXED | MAP_PRIVATE, 0,
+			&current->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(&current->mm->mmap_sem);
 	retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
 	up_write(&current->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(&current->mm->mmap_sem);
 	retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
 	up_write(&current->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

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Kasatkin, Dmitry]

On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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(&current->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(&current->mm->mmap_sem);
> +                               fd_offset, &current->mm->mmap_sem);
>
>                if (error != N_TXTADDR(ex)) {
>                        send_sig(SIGKILL, current, 0);
>                        return error;
>                }
>
> -               down_write(&current->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(&current->mm->mmap_sem);
> +                               fd_offset + ex.a_text, &current->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(&current->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(&current->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),
> +                                &current->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(&current->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(&current->mm->mmap_sem);
> +                       fd_offset, &current->mm->mmap_sem);
>
>                if (error != N_TXTADDR(ex)) {
>                        send_sig(SIGKILL, current, 0);
>                        return error;
>                }
>
> -               down_write(&current->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(&current->mm->mmap_sem);
> +                               fd_offset + ex.a_text, &current->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(&current->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(&current->mm->mmap_sem);
> +                       N_TXTOFF(ex), &current->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(&current->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(&current->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(&current->mm->mmap_sem);
> +                        ELF_PAGEOFFSET(eppnt->p_vaddr)),
> +                       &current->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(&current->mm->mmap_sem);
>                textpos = do_mmap(bprm->file, 0, text_len, PROT_READ|PROT_EXEC,
> -                                 MAP_PRIVATE|MAP_EXECUTABLE, 0);
> -               up_write(&current->mm->mmap_sem);
> +                                 MAP_PRIVATE|MAP_EXECUTABLE, 0,
> +                                 &current->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(&current->mm->mmap_sem);
> -       retval = do_mmap(file, code_start, code_size, prot,
> -                       flags, SOM_PAGESTART(hpuxhdr->exec_tfile));
> -       up_write(&current->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(&current->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(&current->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(&current->mm->mmap_sem);
> -       retval = do_mmap(NULL, bss_start, som_brk - bss_start,
> -                       prot | PROT_WRITE, MAP_FIXED | MAP_PRIVATE, 0);
> -       up_write(&current->mm->mmap_sem);
> +       retval = do_mmap_with_sem(NULL, bss_start, som_brk - bss_start,
> +                       prot | PROT_WRITE, MAP_FIXED | MAP_PRIVATE, 0,
> +                       &current->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(&current->mm->mmap_sem);
>        retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
>        up_write(&current->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(&current->mm->mmap_sem);
>        retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
>        up_write(&current->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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Mimi Zohar]

On Tue, 2011-11-15 at 16:17 +0200, Kasatkin, Dmitry wrote:
> On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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

There are a couple of reasons for deferring IMA processing until
BPRM_CHECK/FILE_MMAP:
- Defer processing until the file has been locked and won't be modified
- Different policies can be associated with the different hooks

For example, with the ima_tcb policy, only files opened for read by root
are measured at file_check, but all files mmapped executable are
measured at file_mmap.  So although a file is opened before it is
mmapped, we don't know apriori if it will be mmapped.  We could allocate
the iint for all inodes opened for read, but that would kind of defeat
the purpose of dynamically allocating the iint as needed.

thanks,

Mimi

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Kasatkin, Dmitry]

On Tue, Nov 15, 2011 at 4:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2011-11-15 at 16:17 +0200, Kasatkin, Dmitry wrote:
>> On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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
>
> There are a couple of reasons for deferring IMA processing until
> BPRM_CHECK/FILE_MMAP:
> - Defer processing until the file has been locked and won't be modified
> - Different policies can be associated with the different hooks
>
> For example, with the ima_tcb policy, only files opened for read by root
> are measured at file_check, but all files mmapped executable are
> measured at file_mmap.  So although a file is opened before it is
> mmapped, we don't know apriori if it will be mmapped.  We could allocate
> the iint for all inodes opened for read, but that would kind of defeat
> the purpose of dynamically allocating the iint as needed.
>

As you are asking for possible alternative solution,
I think I might have one.

It could possibly done in such away:

When binaries or executables are opened for mmap or bprm,
kernel sets open_flag |= __FMODE_EXEC;

ima_file_check() could have additional parameter: op->open_flag
and implementation could selection a function as:
int function = (flag & __FMODE_EXEC) ? BPRM_CHECK : FILE_CHECK;

IMA policy has the same entries for BPRM_CHECK or FILE_MMAP.

This can possibly make mmap and bprm hooks redundant.

- Dmitry


> thanks,
>
> Mimi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Mimi Zohar]

On Tue, 2011-11-15 at 19:05 +0200, Kasatkin, Dmitry wrote:
> On Tue, Nov 15, 2011 at 4:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2011-11-15 at 16:17 +0200, Kasatkin, Dmitry wrote:
> >> On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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
> >
> > There are a couple of reasons for deferring IMA processing until
> > BPRM_CHECK/FILE_MMAP:
> > - Defer processing until the file has been locked and won't be modified
> > - Different policies can be associated with the different hooks
> >
> > For example, with the ima_tcb policy, only files opened for read by root
> > are measured at file_check, but all files mmapped executable are
> > measured at file_mmap.  So although a file is opened before it is
> > mmapped, we don't know apriori if it will be mmapped.  We could allocate
> > the iint for all inodes opened for read, but that would kind of defeat
> > the purpose of dynamically allocating the iint as needed.
> >
> 
> As you are asking for possible alternative solution,
> I think I might have one.
> 
> It could possibly done in such away:
> 
> When binaries or executables are opened for mmap or bprm,
> kernel sets open_flag |= __FMODE_EXEC;
> 
> ima_file_check() could have additional parameter: op->open_flag
> and implementation could selection a function as:
> int function = (flag & __FMODE_EXEC) ? BPRM_CHECK : FILE_CHECK;
> 
> IMA policy has the same entries for BPRM_CHECK or FILE_MMAP.
> 
> This can possibly make mmap and bprm hooks redundant.
> 
> - Dmitry

As a file can be opened read only and then mmapped executable, it is
impossible to know on open, whether that file will be mmapped
executable.

Mimi

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Kasatkin, Dmitry]

On Wed, Nov 16, 2011 at 1:04 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2011-11-15 at 19:05 +0200, Kasatkin, Dmitry wrote:
>> On Tue, Nov 15, 2011 at 4:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > On Tue, 2011-11-15 at 16:17 +0200, Kasatkin, Dmitry wrote:
>> >> On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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
>> >
>> > There are a couple of reasons for deferring IMA processing until
>> > BPRM_CHECK/FILE_MMAP:
>> > - Defer processing until the file has been locked and won't be modified
>> > - Different policies can be associated with the different hooks
>> >
>> > For example, with the ima_tcb policy, only files opened for read by root
>> > are measured at file_check, but all files mmapped executable are
>> > measured at file_mmap.  So although a file is opened before it is
>> > mmapped, we don't know apriori if it will be mmapped.  We could allocate
>> > the iint for all inodes opened for read, but that would kind of defeat
>> > the purpose of dynamically allocating the iint as needed.
>> >
>>
>> As you are asking for possible alternative solution,
>> I think I might have one.
>>
>> It could possibly done in such away:
>>
>> When binaries or executables are opened for mmap or bprm,
>> kernel sets open_flag |= __FMODE_EXEC;
>>
>> ima_file_check() could have additional parameter: op->open_flag
>> and implementation could selection a function as:
>> int function = (flag & __FMODE_EXEC) ? BPRM_CHECK : FILE_CHECK;
>>
>> IMA policy has the same entries for BPRM_CHECK or FILE_MMAP.
>>
>> This can possibly make mmap and bprm hooks redundant.
>>
>> - Dmitry
>
> As a file can be opened read only and then mmapped executable, it is
> impossible to know on open, whether that file will be mmapped
> executable.
>

If the file has been already opened, it has been already verified...

But the point is probably if that while file is opened read-only, it
can be opened "write" by some other process.
It will invalidate verification result and mmap hook would provide
re-verification.

Right?

- Dmitry



> Mimi
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Mimi Zohar]

On Wed, 2011-11-16 at 11:35 +0200, Kasatkin, Dmitry wrote:
> On Wed, Nov 16, 2011 at 1:04 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2011-11-15 at 19:05 +0200, Kasatkin, Dmitry wrote:
> >> On Tue, Nov 15, 2011 at 4:44 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> > On Tue, 2011-11-15 at 16:17 +0200, Kasatkin, Dmitry wrote:
> >> >> On Tue, Nov 15, 2011 at 2:31 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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
> >> >
> >> > There are a couple of reasons for deferring IMA processing until
> >> > BPRM_CHECK/FILE_MMAP:
> >> > - Defer processing until the file has been locked and won't be modified
> >> > - Different policies can be associated with the different hooks
> >> >
> >> > For example, with the ima_tcb policy, only files opened for read by root
> >> > are measured at file_check, but all files mmapped executable are
> >> > measured at file_mmap.  So although a file is opened before it is
> >> > mmapped, we don't know apriori if it will be mmapped.  We could allocate
> >> > the iint for all inodes opened for read, but that would kind of defeat
> >> > the purpose of dynamically allocating the iint as needed.
> >> >
> >>
> >> As you are asking for possible alternative solution,
> >> I think I might have one.
> >>
> >> It could possibly done in such away:
> >>
> >> When binaries or executables are opened for mmap or bprm,
> >> kernel sets open_flag |= __FMODE_EXEC;
> >>
> >> ima_file_check() could have additional parameter: op->open_flag
> >> and implementation could selection a function as:
> >> int function = (flag & __FMODE_EXEC) ? BPRM_CHECK : FILE_CHECK;
> >>
> >> IMA policy has the same entries for BPRM_CHECK or FILE_MMAP.
> >>
> >> This can possibly make mmap and bprm hooks redundant.
> >>
> >> - Dmitry
> >
> > As a file can be opened read only and then mmapped executable, it is
> > impossible to know on open, whether that file will be mmapped
> > executable.
> >
> 
> If the file has been already opened, it has been already verified...

That would imply that everything that is opened for 'read' would have an
iint allocated.  At which point, we might as well go back to using the
inode_alloc hook.

> But the point is probably if that while file is opened read-only, it
> can be opened "write" by some other process.
> It will invalidate verification result and mmap hook would provide
> re-verification.
> 
> Right?
> 
> - Dmitry

Right, so the patch I posted, here, doesn't measure on FILE_PREMMAP, but
only allocates the 'iint' and defers measurement to FILE_MMAP.

Mimi

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Eric Paris]

On Tue, 2011-11-15 at 07:31 -0500, Mimi Zohar 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?

The idea is ok, but I'm not a fan of the patch itself.

> 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);

I don't like the new helper.  I'd much rather just sprinkle
ima_file_premmap() all over the place.  Anything that hides locking
deeper makes me sad.

[snip]
> 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 };

Really don't like this.  Do we really need to extend the language rules
to support this?

>  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;

Lets just break the beginning of this function off into its own helper
function which you use in ima_pre_mmap as well.

> +
>  	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;
> +}

Here lets call the helper above, but instead of FILE_PREMMAP, lets use
the correct FILE_MMAP or FILE_BPRM, which is going to have to come as a
third argument, right?

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Mimi Zohar]

On Wed, 2011-11-16 at 12:27 -0500, Eric Paris wrote:
> On Tue, 2011-11-15 at 07:31 -0500, Mimi Zohar 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?
> 
> The idea is ok, but I'm not a fan of the patch itself.

np, neither am I.  I was hoping that there was a better overall
approach. :-(  If not, then I'll clean up this patch.

> > 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);
> 
> I don't like the new helper.  I'd much rather just sprinkle
> ima_file_premmap() all over the place.  Anything that hides locking
> deeper makes me sad.

Either way is painful.

> [snip]
> > 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 };
> 
> Really don't like this.  Do we really need to extend the language rules
> to support this?

No, with the right refactoring it's probably unnecessary.  In fact, we
could land up with policy consistency errors.

> >  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;
> 
> Lets just break the beginning of this function off into its own helper
> function which you use in ima_pre_mmap as well.

Right

> > +
> >  	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;
> > +}
> 
> Here lets call the helper above, but instead of FILE_PREMMAP, lets use
> the correct FILE_MMAP or FILE_BPRM, which is going to have to come as a
> third argument, right?

Ok, thanks for the review.

Mimi

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Eric Paris]

On Wed, 2011-11-16 at 15:24 -0500, Mimi Zohar wrote:
> On Wed, 2011-11-16 at 12:27 -0500, Eric Paris wrote:

> > > +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;
> > > +}
> > 
> > Here lets call the helper above, but instead of FILE_PREMMAP, lets use
> > the correct FILE_MMAP or FILE_BPRM, which is going to have to come as a
> > third argument, right?
> 
> Ok, thanks for the review.

Actually it just dawned on me, that what I think we really learned here
is that we have the hooks in the wrong place.  If we are going to
sprinkle the code with ima_file_premmap() maybe we should just actually
do ALL of the work in this new hook?  Throw the security_file_mmap() IMA
hook out the window and just actually do the whole process_measurement()
work in this new hook?    You'll have to see if that's better.  But it
sucks to allocate an object just to have to look it up 100 instructions
later.  Why not just do the work when we do the allocation and not come
back a second time?

-Eric

Re: [RFC][PATCH] ima: fix lockdep circular locking dependency

[Mimi Zohar]

On Wed, 2011-11-16 at 15:49 -0500, Eric Paris wrote:
> On Wed, 2011-11-16 at 15:24 -0500, Mimi Zohar wrote:
> > On Wed, 2011-11-16 at 12:27 -0500, Eric Paris wrote:
> 
> > > > +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;
> > > > +}
> > > 
> > > Here lets call the helper above, but instead of FILE_PREMMAP, lets use
> > > the correct FILE_MMAP or FILE_BPRM, which is going to have to come as a
> > > third argument, right?
> > 
> > Ok, thanks for the review.
> 
> Actually it just dawned on me, that what I think we really learned here
> is that we have the hooks in the wrong place.  If we are going to
> sprinkle the code with ima_file_premmap() maybe we should just actually
> do ALL of the work in this new hook?  Throw the security_file_mmap() IMA
> hook out the window and just actually do the whole process_measurement()
> work in this new hook?    You'll have to see if that's better.  But it
> sucks to allocate an object just to have to look it up 100 instructions
> later.  Why not just do the work when we do the allocation and not come
> back a second time?
> 
> -Eric

We want to wait until the file is locked and can't be modified before we
measure it.  Am looking for where that is done.

Mimi