Re: [PATCH] vfs: fix IMA lockdep circular locking dependency

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Tue, May 15, 2012 at 5:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Er...  Looks like you forgot to actually put the patch in there ;-)

Oops. Err.. I meant to do that. Right. To check that you're awake.

Here.

                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 18211 bytes --]

 fs/binfmt_elf.c            |  6 ++++++
 fs/exec.c                  |  2 +-
 include/linux/security.h   | 41 +++++++++++++++++++++--------------------
 mm/mmap.c                  | 17 ++++++++++++-----
 mm/mremap.c                |  4 ++--
 mm/nommu.c                 | 17 +++++++++++------
 security/apparmor/lsm.c    | 14 +++-----------
 security/capability.c      |  1 -
 security/commoncap.c       | 13 +++----------
 security/security.c        | 37 ++++++++++++++++++++++++++++++-------
 security/selinux/hooks.c   | 33 +++++++++++++++------------------
 security/smack/smack_lsm.c | 21 ++++-----------------
 12 files changed, 108 insertions(+), 98 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 16f735417072..364990a81474 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -321,6 +321,8 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
 	unsigned long map_addr;
 	unsigned long size = eppnt->p_filesz + ELF_PAGEOFFSET(eppnt->p_vaddr);
 	unsigned long off = eppnt->p_offset - ELF_PAGEOFFSET(eppnt->p_vaddr);
+	int ret;
+
 	addr = ELF_PAGESTART(addr);
 	size = ELF_PAGEALIGN(size);
 
@@ -329,6 +331,10 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
 	if (!size)
 		return addr;
 
+	ret = security_mmap_file(filep, prot, type);
+	if (ret)
+		return ret;
+
 	down_write(&current->mm->mmap_sem);
 	/*
 	* total_size is the size of the ELF (interpreter) image.
diff --git a/fs/exec.c b/fs/exec.c
index b1fd2025e59a..9ffce472e759 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -280,7 +280,7 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
 
-	err = security_file_mmap(NULL, 0, 0, 0, vma->vm_start, 1);
+	err = security_mmap_addr(vma->vm_start);
 	if (err)
 		goto err;
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 673afbb8238a..52f829d1cd4e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -86,9 +86,7 @@ extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
 extern int cap_inode_removexattr(struct dentry *dentry, const char *name);
 extern int cap_inode_need_killpriv(struct dentry *dentry);
 extern int cap_inode_killpriv(struct dentry *dentry);
-extern int cap_file_mmap(struct file *file, unsigned long reqprot,
-			 unsigned long prot, unsigned long flags,
-			 unsigned long addr, unsigned long addr_only);
+extern int cap_mmap_addr(unsigned long addr);
 extern int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags);
 extern int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			  unsigned long arg4, unsigned long arg5);
@@ -585,15 +583,16 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	simple integer value.  When @arg represents a user space pointer, it
  *	should never be used by the security module.
  *	Return 0 if permission is granted.
- * @file_mmap :
- *	Check permissions for a mmap operation.  The @file may be NULL, e.g.
- *	if mapping anonymous memory.
- *	@file contains the file structure for file to map (may be NULL).
+ * @mmap_file :
+ *	Check permissions for a mmap operation.  The @file will not be NULL
+ *	@file contains the file structure for file to map.
  *	@reqprot contains the protection requested by the application.
  *	@prot contains the protection that will be applied by the kernel.
  *	@flags contains the operational flags.
- *	@addr contains virtual address that will be used for the operation.
- *	@addr_only contains a boolean: 0 if file-backed VMA, otherwise 1.
+ *	Return 0 if permission is granted.
+ * @mmap_addr :
+ *	Check permissions for a mmap operation.
+ *	@addr contains the address to be mmaped
  *	Return 0 if permission is granted.
  * @file_mprotect:
  *	Check permissions before changing memory access permissions.
@@ -1483,10 +1482,10 @@ struct security_operations {
 	void (*file_free_security) (struct file *file);
 	int (*file_ioctl) (struct file *file, unsigned int cmd,
 			   unsigned long arg);
-	int (*file_mmap) (struct file *file,
+	int (*mmap_file) (struct file *file,
 			  unsigned long reqprot, unsigned long prot,
-			  unsigned long flags, unsigned long addr,
-			  unsigned long addr_only);
+			  unsigned long flags);
+	int (*mmap_addr) (unsigned long addr);
 	int (*file_mprotect) (struct vm_area_struct *vma,
 			      unsigned long reqprot,
 			      unsigned long prot);
@@ -1745,9 +1744,8 @@ int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
-int security_file_mmap(struct file *file, unsigned long reqprot,
-			unsigned long prot, unsigned long flags,
-			unsigned long addr, unsigned long addr_only);
+int security_mmap_file(struct file *file, unsigned long reqprot, unsigned long flags);
+int security_mmap_addr(unsigned long addr);
 int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 			   unsigned long prot);
 int security_file_lock(struct file *file, unsigned int cmd);
@@ -2183,13 +2181,16 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
 	return 0;
 }
 
-static inline int security_file_mmap(struct file *file, unsigned long reqprot,
+static inline int security_mmap_file(struct file *file,
 				     unsigned long prot,
-				     unsigned long flags,
-				     unsigned long addr,
-				     unsigned long addr_only)
+				     unsigned long flags)
+{
+	return 0;
+}
+
+static inline int security_mmap_addr(unsigned long addr)
 {
-	return cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
+	return cap_mmap_addr(addr);
 }
 
 static inline int security_file_mprotect(struct vm_area_struct *vma,
diff --git a/mm/mmap.c b/mm/mmap.c
index 848ef52d9603..db2626527a7e 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -961,7 +961,6 @@ static unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	struct inode *inode;
 	vm_flags_t vm_flags;
 	int error;
-	unsigned long reqprot = prot;
 
 	/*
 	 * Does the application expect PROT_READ to imply PROT_EXEC?
@@ -1083,7 +1082,7 @@ static unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 		}
 	}
 
-	error = security_file_mmap(file, reqprot, prot, flags, addr, 0);
+	error = security_mmap_addr(addr);
 	if (error)
 		return error;
 
@@ -1109,6 +1108,10 @@ unsigned long vm_mmap(struct file *file, unsigned long addr,
 	unsigned long ret;
 	struct mm_struct *mm = current->mm;
 
+	ret = security_mmap_file(file, prot, flag);
+	if (ret)
+		return ret;
+
 	down_write(&mm->mmap_sem);
 	ret = do_mmap(file, addr, len, prot, flag, offset);
 	up_write(&mm->mmap_sem);
@@ -1147,6 +1150,10 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 
 	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
 
+	retval = security_mmap_file(file, prot, flags);
+	if (retval)
+		goto out;
+
 	down_write(&current->mm->mmap_sem);
 	retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
 	up_write(&current->mm->mmap_sem);
@@ -1795,7 +1802,7 @@ int expand_downwards(struct vm_area_struct *vma,
 		return -ENOMEM;
 
 	address &= PAGE_MASK;
-	error = security_file_mmap(NULL, 0, 0, 0, address, 1);
+	error = security_mmap_addr(address);
 	if (error)
 		return error;
 
@@ -2180,7 +2187,7 @@ static unsigned long do_brk(unsigned long addr, unsigned long len)
 	if (!len)
 		return addr;
 
-	error = security_file_mmap(NULL, 0, 0, 0, addr, 1);
+	error = security_mmap_addr(addr);
 	if (error)
 		return error;
 
@@ -2525,7 +2532,7 @@ int install_special_mapping(struct mm_struct *mm,
 	vma->vm_ops = &special_mapping_vmops;
 	vma->vm_private_data = pages;
 
-	ret = security_file_mmap(NULL, 0, 0, 0, vma->vm_start, 1);
+	ret = security_mmap_addr(vma->vm_start);
 	if (ret)
 		goto out;
 
diff --git a/mm/mremap.c b/mm/mremap.c
index db8d983b5a7d..68f6b14223e5 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -371,7 +371,7 @@ static unsigned long mremap_to(unsigned long addr,
 	if ((addr <= new_addr) && (addr+old_len) > new_addr)
 		goto out;
 
-	ret = security_file_mmap(NULL, 0, 0, 0, new_addr, 1);
+	ret = security_mmap_addr(new_addr);
 	if (ret)
 		goto out;
 
@@ -530,7 +530,7 @@ unsigned long do_mremap(unsigned long addr,
 			goto out;
 		}
 
-		ret = security_file_mmap(NULL, 0, 0, 0, new_addr, 1);
+		ret = security_mmap_addr(new_addr);
 		if (ret)
 			goto out;
 		ret = move_vma(vma, addr, old_len, new_len, new_addr);
diff --git a/mm/nommu.c b/mm/nommu.c
index bb8f4f004a82..f29151a23609 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -889,7 +889,6 @@ static int validate_mmap_request(struct file *file,
 				 unsigned long *_capabilities)
 {
 	unsigned long capabilities, rlen;
-	unsigned long reqprot = prot;
 	int ret;
 
 	/* do the simple checks first */
@@ -1046,8 +1045,7 @@ static int validate_mmap_request(struct file *file,
 			prot |= PROT_EXEC;
 	}
 
-	/* allow the security API to have its say */
-	ret = security_file_mmap(file, reqprot, prot, flags, addr, 0);
+	ret = security_mmap_addr(addr);
 	if (ret < 0)
 		return ret;
 
@@ -1490,6 +1488,10 @@ unsigned long vm_mmap(struct file *file, unsigned long addr,
 	unsigned long ret;
 	struct mm_struct *mm = current->mm;
 
+	ret = security_mmap_file(file, prot, flag);
+	if (ret)
+		return ret;
+
 	down_write(&mm->mmap_sem);
 	ret = do_mmap(file, addr, len, prot, flag, offset);
 	up_write(&mm->mmap_sem);
@@ -1513,9 +1515,12 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 
 	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
 
-	down_write(&current->mm->mmap_sem);
-	retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
-	up_write(&current->mm->mmap_sem);
+	retval = security_mmap_file(file, prot, flags);
+	if (!retval) {
+		down_write(&current->mm->mmap_sem);
+		retval = do_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+		up_write(&current->mm->mmap_sem);
+	}
 
 	if (file)
 		fput(file);
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index ad05d391974d..6f94147a0c3b 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -490,17 +490,9 @@ static int common_mmap(int op, struct file *file, unsigned long prot,
 	return common_file_perm(op, file, mask);
 }
 
-static int apparmor_file_mmap(struct file *file, unsigned long reqprot,
-			      unsigned long prot, unsigned long flags,
-			      unsigned long addr, unsigned long addr_only)
+static int apparmor_mmap_file(struct file *file, unsigned long reqprot,
+			      unsigned long prot, unsigned long flags)
 {
-	int rc = 0;
-
-	/* do DAC check */
-	rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
-	if (rc || addr_only)
-		return rc;
-
 	return common_mmap(OP_FMMAP, file, prot, flags);
 }
 
@@ -646,7 +638,7 @@ static struct security_operations apparmor_ops = {
 	.file_permission =		apparmor_file_permission,
 	.file_alloc_security =		apparmor_file_alloc_security,
 	.file_free_security =		apparmor_file_free_security,
-	.file_mmap =			apparmor_file_mmap,
+	.mmap_file =			apparmor_mmap_file,
 	.file_mprotect =		apparmor_file_mprotect,
 	.file_lock =			apparmor_file_lock,
 
diff --git a/security/capability.c b/security/capability.c
index 5bb21b1c448c..9a19c6a54e12 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -949,7 +949,6 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, file_alloc_security);
 	set_to_cap_if_null(ops, file_free_security);
 	set_to_cap_if_null(ops, file_ioctl);
-	set_to_cap_if_null(ops, file_mmap);
 	set_to_cap_if_null(ops, file_mprotect);
 	set_to_cap_if_null(ops, file_lock);
 	set_to_cap_if_null(ops, file_fcntl);
diff --git a/security/commoncap.c b/security/commoncap.c
index 71a166a05975..faf105ab0474 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -944,22 +944,15 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
 }
 
 /*
- * cap_file_mmap - check if able to map given addr
- * @file: unused
- * @reqprot: unused
- * @prot: unused
- * @flags: unused
- * @addr: address attempting to be mapped
- * @addr_only: unused
+ * cap_mmap_addr - check if able to map given addr
+ * @addr: address mmap'ed
  *
  * If the process is attempting to map memory below dac_mmap_min_addr they need
  * CAP_SYS_RAWIO.  The other parameters to this function are unused by the
  * capability security module.  Returns 0 if this mapping should be allowed
  * -EPERM if not.
  */
-int cap_file_mmap(struct file *file, unsigned long reqprot,
-		  unsigned long prot, unsigned long flags,
-		  unsigned long addr, unsigned long addr_only)
+int cap_mmap_addr(unsigned long addr)
 {
 	int ret = 0;
 
diff --git a/security/security.c b/security/security.c
index bf619ffc9a4d..8fea6a0750a2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -20,6 +20,8 @@
 #include <linux/ima.h>
 #include <linux/evm.h>
 #include <linux/fsnotify.h>
+#include <linux/mman.h>
+#include <linux/mount.h>
 #include <net/flow.h>
 
 #define MAX_LSM_EVM_XATTR	2
@@ -657,18 +659,39 @@ int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return security_ops->file_ioctl(file, cmd, arg);
 }
 
-int security_file_mmap(struct file *file, unsigned long reqprot,
-			unsigned long prot, unsigned long flags,
-			unsigned long addr, unsigned long addr_only)
+int security_mmap_file(struct file *file, unsigned long reqprot, unsigned long flags)
 {
-	int ret;
+	unsigned long prot;
 
-	ret = security_ops->file_mmap(file, reqprot, prot, flags, addr, addr_only);
-	if (ret)
-		return ret;
+	if (!file)
+		return 0;
+
+	/* Does the application expect PROT_READ to imply PROT_EXEC? */
+	prot = reqprot;
+	if ((prot & (PROT_READ | PROT_EXEC)) == PROT_READ) {
+		if (current->personality & READ_IMPLIES_EXEC)
+			if (!(file->f_path.mnt->mnt_flags & MNT_NOEXEC))
+				prot |= PROT_EXEC;
+	}
+
+	if (security_ops->mmap_file) {
+		int ret = security_ops->mmap_file(file, reqprot, prot, flags);
+		if (ret)
+			return ret;
+	}
 	return ima_file_mmap(file, prot);
 }
 
+int security_mmap_addr(unsigned long addr)
+{
+	if (security_ops->mmap_addr) {
+		int ret = security_ops->mmap_addr(addr);
+		if (ret)
+			return ret;
+	}
+	return cap_mmap_addr(addr);
+}
+
 int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 			    unsigned long prot)
 {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d85b793c9321..f911e61c152e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3069,31 +3069,27 @@ error:
 	return rc;
 }
 
-static int selinux_file_mmap(struct file *file, unsigned long reqprot,
-			     unsigned long prot, unsigned long flags,
-			     unsigned long addr, unsigned long addr_only)
+/*
+ * notice that the security layer is intentionally doing the SELinux
+ * check before the secondary cap_mmap_file check.  This is such a
+ * likely attempt at bad behaviour/exploit that we always want to get
+ * the AVC, even if DAC would have also denied the operation.
+ */
+static int selinux_mmap_addr(unsigned long addr)
 {
 	int rc = 0;
-	u32 sid = current_sid();
 
-	/*
-	 * notice that we are intentionally putting the SELinux check before
-	 * the secondary cap_file_mmap check.  This is such a likely attempt
-	 * at bad behaviour/exploit that we always want to get the AVC, even
-	 * if DAC would have also denied the operation.
-	 */
 	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
+		u32 sid = current_sid();
 		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
 				  MEMPROTECT__MMAP_ZERO, NULL);
-		if (rc)
-			return rc;
 	}
+	return rc;
+}
 
-	/* do DAC check on address space usage */
-	rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
-	if (rc || addr_only)
-		return rc;
-
+static int selinux_mmap_file(struct file *file, unsigned long reqprot,
+			     unsigned long prot, unsigned long flags)
+{
 	if (selinux_checkreqprot)
 		prot = reqprot;
 
@@ -5584,7 +5580,8 @@ static struct security_operations selinux_ops = {
 	.file_alloc_security =		selinux_file_alloc_security,
 	.file_free_security =		selinux_file_free_security,
 	.file_ioctl =			selinux_file_ioctl,
-	.file_mmap =			selinux_file_mmap,
+	.mmap_file =			selinux_mmap_file,
+	.mmap_addr =			selinux_mmap_addr,
 	.file_mprotect =		selinux_file_mprotect,
 	.file_lock =			selinux_file_lock,
 	.file_fcntl =			selinux_file_fcntl,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 45c32f074166..40960052fc64 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1161,7 +1161,7 @@ static int smack_file_fcntl(struct file *file, unsigned int cmd,
 }
 
 /**
- * smack_file_mmap :
+ * smack_mmap_file :
  * Check permissions for a mmap operation.  The @file may be NULL, e.g.
  * if mapping anonymous memory.
  * @file contains the file structure for file to map (may be NULL).
@@ -1170,10 +1170,9 @@ static int smack_file_fcntl(struct file *file, unsigned int cmd,
  * @flags contains the operational flags.
  * Return 0 if permission is granted.
  */
-static int smack_file_mmap(struct file *file,
+static int smack_mmap_file(struct file *file,
 			   unsigned long reqprot, unsigned long prot,
-			   unsigned long flags, unsigned long addr,
-			   unsigned long addr_only)
+			   unsigned long flags)
 {
 	struct smack_known *skp;
 	struct smack_rule *srp;
@@ -1188,19 +1187,7 @@ static int smack_file_mmap(struct file *file,
 	int tmay;
 	int rc;
 
-	/* do DAC check on address space usage */
-	rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
-	if (rc || addr_only)
-		return rc;
-
-	if (file == NULL || file->f_dentry == NULL)
-		return 0;
-
 	dp = file->f_dentry;
-
-	if (dp->d_inode == NULL)
-		return 0;
-
 	isp = dp->d_inode->i_security;
 	if (isp->smk_mmap == NULL)
 		return 0;
@@ -3544,7 +3531,7 @@ struct security_operations smack_ops = {
 	.file_ioctl = 			smack_file_ioctl,
 	.file_lock = 			smack_file_lock,
 	.file_fcntl = 			smack_file_fcntl,
-	.file_mmap =			smack_file_mmap,
+	.mmap_file =			smack_mmap_file,
 	.file_set_fowner = 		smack_file_set_fowner,
 	.file_send_sigiotask = 		smack_file_send_sigiotask,
 	.file_receive = 		smack_file_receive,