Date:	Fri, 16 May 2014 15:03:19 +0300
From:	Dmitry Kasatkin <d.kasatkin@...sung.com>
To:	viro@...iv.linux.org.uk, linux-security-module@...r.kernel.org,
	eparis@...hat.com, zohar@...ux.vnet.ibm.com,
	linux-ima-devel@...ts.sourceforge.net
Cc:	linux-kernel@...r.kernel.org, jmorris@...ei.org,
	dmitry.kasatkin@...il.com, Dmitry Kasatkin <d.kasatkin@...sung.com>
Subject: [PATCHv2 1/1] ima: re-introduce own integrity cache lock

Before IMA appraisal was introduced, IMA was using own integrity cache
lock along with i_mutex. process_measurement and ima_file_free took
the iint->mutex first and then the i_mutex, while setxattr, chmod and
chown took the locks in reverse order. To resolve the potential deadlock,
i_mutex was moved to protect entire IMA functionality and the redundant
iint->mutex was eliminated.

Solution was based on the assumption that filesystem code does not take
i_mutex further. But when file is opened with O_DIRECT flag, direct-io
implementation takes i_mutex and produces deadlock. Furthermore, certain
other filesystem operations, such as llseek, also take i_mutex.

To resolve O_DIRECT related deadlock problem, this patch re-introduces
iint->mutex. But to eliminate the original chmod() related deadlock
problem, this patch eliminates the requirement for chmod hooks to take
the iint->mutex by introducing additional atomic iint->attr_flags to
indicate calling of the hooks. The allowed locking order is to take
the iint->mutex first and then the i_mutex.

Changes to v1:
* revert taking the i_mutex in integrity_inode_get() so that iint allocation
  could be done with i_mutex taken
* move taking the i_mutex from appraisal code to the process_measurement()

Signed-off-by: Dmitry Kasatkin <d.kasatkin@...sung.com>
---
 security/integrity/iint.c             |  2 ++
 security/integrity/ima/ima_appraise.c | 20 ++++++--------------
 security/integrity/ima/ima_main.c     | 32 +++++++++++++++++++++-----------
 security/integrity/integrity.h        |  5 +++++
 4 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index a521edf..d293207 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -154,11 +154,13 @@ static void init_once(void *foo)
 	memset(iint, 0, sizeof(*iint));
 	iint->version = 0;
 	iint->flags = 0UL;
+	iint->attr_flags = 0;
 	iint->ima_file_status = INTEGRITY_UNKNOWN;
 	iint->ima_mmap_status = INTEGRITY_UNKNOWN;
 	iint->ima_bprm_status = INTEGRITY_UNKNOWN;
 	iint->ima_module_status = INTEGRITY_UNKNOWN;
 	iint->evm_status = INTEGRITY_UNKNOWN;
+	mutex_init(&iint->mutex);
 }
 
 static int __init integrity_iintcache_init(void)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index d3113d4..c49f8c3 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -289,7 +289,9 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file)
 	if (rc < 0)
 		return;
 
+	mutex_lock(&file_inode(file)->i_mutex);
 	ima_fix_xattr(dentry, iint);
+	mutex_unlock(&file_inode(file)->i_mutex);
 }
 
 /**
@@ -313,13 +315,8 @@ void ima_inode_post_setattr(struct dentry *dentry)
 
 	must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
 	iint = integrity_iint_find(inode);
-	if (iint) {
-		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
-				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
-				 IMA_ACTION_FLAGS);
-		if (must_appraise)
-			iint->flags |= IMA_APPRAISE;
-	}
+	if (iint)
+		set_bit(IMA_CHANGE_ATTR, &iint->attr_flags);
 	if (!must_appraise)
 		rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
 	return;
@@ -349,13 +346,8 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
 		return;
 
 	iint = integrity_iint_find(inode);
-	if (!iint)
-		return;
-
-	iint->flags &= ~IMA_DONE_MASK;
-	if (digsig)
-		iint->flags |= IMA_DIGSIG;
-	return;
+	if (iint)
+		set_bit(IMA_CHANGE_XATTR, &iint->attr_flags);
 }
 
 int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index bd7b4cb..fdd5320 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -88,8 +88,6 @@ static void ima_rdwr_violation_check(struct file *file)
 	if (!S_ISREG(inode->i_mode) || !ima_initialized)
 		return;
 
-	mutex_lock(&inode->i_mutex);	/* file metadata: permissions, xattr */
-
 	if (mode & FMODE_WRITE) {
 		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
 			struct integrity_iint_cache *iint;
@@ -104,8 +102,6 @@ static void ima_rdwr_violation_check(struct file *file)
 			send_writers = true;
 	}
 
-	mutex_unlock(&inode->i_mutex);
-
 	if (!send_tomtou && !send_writers)
 		return;
 
@@ -127,14 +123,14 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 	if (!(mode & FMODE_WRITE))
 		return;
 
-	mutex_lock(&inode->i_mutex);
+	mutex_lock(&iint->mutex);
 	if (atomic_read(&inode->i_writecount) == 1 &&
 	    iint->version != inode->i_version) {
 		iint->flags &= ~IMA_DONE_MASK;
 		if (iint->flags & IMA_APPRAISE)
 			ima_update_xattr(iint, file);
 	}
-	mutex_unlock(&inode->i_mutex);
+	mutex_unlock(&iint->mutex);
 }
 
 /**
@@ -187,10 +183,21 @@ static int process_measurement(struct file *file, const char *filename,
 	_func = (action & IMA_FILE_APPRAISE) ? FILE_CHECK : function;
 
 	mutex_lock(&inode->i_mutex);
-
 	iint = integrity_inode_get(inode);
+	mutex_unlock(&inode->i_mutex);
 	if (!iint)
-		goto out;
+		goto out_unlocked;
+
+	mutex_lock(&iint->mutex);
+
+	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->attr_flags))
+		/* reset appraisal flags if ima_inode_post_setattr was called */
+		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
+				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK);
+
+	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->attr_flags))
+		/* reset all flags if ima_inode_setxattr was called */
+		iint->flags &= ~IMA_DONE_MASK;
 
 	/* Determine if already appraised/measured based on bitmask
 	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
@@ -225,18 +232,21 @@ static int process_measurement(struct file *file, const char *filename,
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len);
-	if (action & IMA_APPRAISE_SUBMASK)
+	if (action & IMA_APPRAISE_SUBMASK) {
+		mutex_lock(&inode->i_mutex);
 		rc = ima_appraise_measurement(_func, iint, file, pathname,
 					      xattr_value, xattr_len);
+		mutex_unlock(&inode->i_mutex);
+	}
 	if (action & IMA_AUDIT)
 		ima_audit_measurement(iint, pathname);
 	kfree(pathbuf);
 out_digsig:
 	if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG))
 		rc = -EACCES;
-out:
-	mutex_unlock(&inode->i_mutex);
+	mutex_unlock(&iint->mutex);
 	kfree(xattr_value);
+out_unlocked:
 	if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
 		return -EACCES;
 	return 0;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 2fb5e53..f73cd06 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -50,6 +50,9 @@
 #define IMA_APPRAISED_SUBMASK	(IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \
 				 IMA_BPRM_APPRAISED | IMA_MODULE_APPRAISED)
 
+#define IMA_CHANGE_XATTR	0
+#define IMA_CHANGE_ATTR		1
+
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
 	EVM_XATTR_HMAC,
@@ -96,9 +99,11 @@ struct signature_v2_hdr {
 /* integrity data associated with an inode */
 struct integrity_iint_cache {
 	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
+	struct mutex mutex;	/* protects: version, flags, digest */
 	struct inode *inode;	/* back pointer to inode in question */
 	u64 version;		/* track inode changes */
 	unsigned long flags;
+	unsigned long attr_flags;
 	enum integrity_status ima_file_status:4;
 	enum integrity_status ima_mmap_status:4;
 	enum integrity_status ima_bprm_status:4;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists