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 in v4:
* adoped to violation detection fixes
* added IMA_UPDATE_XATTR flag to require xattr update on file close
Changes in v3:
* prevent signature removal with new locking
* rename attr_flags to atomic_flags
Changes in v2:
* 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 <dmitry.kasatkin@huawei.com>
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 2de9c82..eb6b0f4 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -154,11 +154,13 @@
memset(iint, 0, sizeof(*iint));
iint->version = 0;
iint->flags = 0UL;
+ iint->atomic_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 1873b55..3ac5780 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -229,6 +229,7 @@
status = INTEGRITY_FAIL;
break;
}
+ clear_bit(IMA_DIGSIG, &iint->atomic_flags);
if (xattr_len - sizeof(xattr_value->type) - hash_start >=
iint->ima_hash->length)
/* xattr length may be longer. md5 hash in previous
@@ -247,7 +248,7 @@
status = INTEGRITY_PASS;
break;
case EVM_IMA_XATTR_DIGSIG:
- iint->flags |= IMA_DIGSIG;
+ set_bit(IMA_DIGSIG, &iint->atomic_flags);
rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
(const char *)xattr_value, rc,
iint->ima_hash->digest,
@@ -293,14 +294,16 @@
int rc = 0;
/* do not collect and update hash for digital signatures */
- if (iint->flags & IMA_DIGSIG)
+ if (test_bit(IMA_DIGSIG, &iint->atomic_flags))
return;
rc = ima_collect_measurement(iint, file, NULL, NULL);
if (rc < 0)
return;
+ mutex_lock(&file_inode(file)->i_mutex);
ima_fix_xattr(dentry, iint);
+ mutex_unlock(&file_inode(file)->i_mutex);
}
/**
@@ -316,24 +319,21 @@
{
struct inode *inode = d_backing_inode(dentry);
struct integrity_iint_cache *iint;
- int must_appraise, rc;
+ int must_appraise;
if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)
|| !inode->i_op->removexattr)
return;
must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR);
+ if (!must_appraise)
+ inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
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;
+ set_bit(IMA_CHANGE_ATTR, &iint->atomic_flags);
+ if (!must_appraise)
+ clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
}
- if (!must_appraise)
- rc = inode->i_op->removexattr(dentry, XATTR_NAME_IMA);
- return;
}
/*
@@ -362,11 +362,11 @@
iint = integrity_iint_find(inode);
if (!iint)
return;
-
- iint->flags &= ~IMA_DONE_MASK;
+ set_bit(IMA_CHANGE_XATTR, &iint->atomic_flags);
if (digsig)
- iint->flags |= IMA_DIGSIG;
- return;
+ set_bit(IMA_DIGSIG, &iint->atomic_flags);
+ else
+ clear_bit(IMA_DIGSIG, &iint->atomic_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 c21f09b..6a2c111 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -117,20 +117,23 @@
struct inode *inode, struct file *file)
{
fmode_t mode = file->f_mode;
+ bool update;
if (!(mode & FMODE_WRITE))
return;
- mutex_lock(&inode->i_mutex);
+ mutex_lock(&iint->mutex);
if (atomic_read(&inode->i_writecount) == 1) {
+ update = test_and_clear_bit(IMA_UPDATE_XATTR,
+ &iint->atomic_flags);
if ((iint->version != inode->i_version) ||
(iint->flags & IMA_NEW_FILE)) {
iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
- if (iint->flags & IMA_APPRAISE)
+ if (update)
ima_update_xattr(iint, file);
}
}
- mutex_unlock(&inode->i_mutex);
+ mutex_unlock(&iint->mutex);
}
/**
@@ -162,7 +165,7 @@
struct ima_template_desc *template_desc;
char *pathbuf = NULL;
const char *pathname = NULL;
- int rc = -ENOMEM, action, must_appraise;
+ int rc = 0, action, must_appraise = 0;
struct evm_ima_xattr_data *xattr_value = NULL, **xattr_ptr = NULL;
int xattr_len = 0;
bool violation_check;
@@ -191,17 +194,31 @@
if (action) {
iint = integrity_inode_get(inode);
if (!iint)
- goto out;
+ rc = -ENOMEM;
}
- if (violation_check) {
+ if (!rc && violation_check)
ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
&pathbuf, &pathname);
- if (!action) {
- rc = 0;
- goto out_free;
- }
- }
+
+ mutex_unlock(&inode->i_mutex);
+
+ if (rc)
+ goto out;
+ if (!action)
+ goto out;
+
+ mutex_lock(&iint->mutex);
+
+ if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
+ /* reset appraisal flags if ima_inode_post_setattr was called */
+ iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
+ IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
+ IMA_ACTION_FLAGS);
+
+ if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_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,
@@ -215,7 +232,7 @@
if (!action) {
if (must_appraise)
rc = ima_get_cache_status(iint, function);
- goto out_digsig;
+ goto out_locked;
}
template_desc = ima_template_desc_current();
@@ -227,7 +244,7 @@
if (rc != 0) {
if (file->f_flags & O_DIRECT)
rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
- goto out_digsig;
+ goto out_locked;
}
if (!pathname) /* ima_rdwr_violation possibly pre-fetched */
@@ -236,23 +253,28 @@
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(function, iint, file, pathname,
xattr_value, xattr_len, opened);
+ mutex_unlock(&inode->i_mutex);
+ }
if (action & IMA_AUDIT)
ima_audit_measurement(iint, pathname);
-
-out_digsig:
- if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG))
+out_locked:
+ if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags))
rc = -EACCES;
+ mutex_unlock(&iint->mutex);
kfree(xattr_value);
-out_free:
+out:
if (pathbuf)
__putname(pathbuf);
-out:
- mutex_unlock(&inode->i_mutex);
- if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE))
- return -EACCES;
+ if (must_appraise) {
+ if (rc && (ima_appraise & IMA_APPRAISE_ENFORCE))
+ return -EACCES;
+ if (file->f_mode & FMODE_WRITE)
+ set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
+ }
return 0;
}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 5efe2ec..37e48e9 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -28,10 +28,9 @@
/* iint cache flags */
#define IMA_ACTION_FLAGS 0xff000000
-#define IMA_DIGSIG 0x01000000
-#define IMA_DIGSIG_REQUIRED 0x02000000
-#define IMA_PERMIT_DIRECTIO 0x04000000
-#define IMA_NEW_FILE 0x08000000
+#define IMA_DIGSIG_REQUIRED 0x01000000
+#define IMA_PERMIT_DIRECTIO 0x02000000
+#define IMA_NEW_FILE 0x04000000
#define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
IMA_APPRAISE_SUBMASK)
@@ -56,6 +55,12 @@
IMA_BPRM_APPRAISED | IMA_MODULE_APPRAISED | \
IMA_FIRMWARE_APPRAISED)
+/* iint cache atomic_flags */
+#define IMA_CHANGE_XATTR 0
+#define IMA_UPDATE_XATTR 1
+#define IMA_CHANGE_ATTR 2
+#define IMA_DIGSIG 3
+
enum evm_ima_xattr_type {
IMA_XATTR_DIGEST = 0x01,
EVM_XATTR_HMAC,
@@ -103,9 +108,11 @@
/* 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 atomic_flags;
enum integrity_status ima_file_status:4;
enum integrity_status ima_mmap_status:4;
enum integrity_status ima_bprm_status:4;
