diff options
author | Dmitry Kasatkin <dmitry.kasatkin@huawei.com> | 2015-10-22 21:26:52 +0300 |
---|---|---|
committer | Dmitry Kasatkin <dmitry.kasatkin@huawei.com> | 2015-10-26 21:13:02 +0200 |
commit | bfcf36341c229b3912bad3b899358abc71836792 (patch) | |
tree | 6c9c5c64eb91e2118398f118015dadee365a3479 | |
parent | 1efd8fde4fe2cb39da1147e8442379d0e98abd1f (diff) | |
download | linux-digsig-ima-next-old.tar.gz |
ima: re-introduce own integrity cache lockima-next-old
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>
-rw-r--r-- | security/integrity/iint.c | 2 | ||||
-rw-r--r-- | security/integrity/ima/ima_appraise.c | 30 | ||||
-rw-r--r-- | security/integrity/ima/ima_main.c | 66 | ||||
-rw-r--r-- | security/integrity/integrity.h | 15 |
4 files changed, 72 insertions, 41 deletions
diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 2de9c820903f1..eb6b0f4602106 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->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 1873b5536f804..3ac578084a886 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -229,6 +229,7 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint, 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 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint, 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 @@ void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file) 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 @@ void ima_inode_post_setattr(struct dentry *dentry) { 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 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig) 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 c21f09bf8b992..6a2c11189a697 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -117,20 +117,23 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint, 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 @@ static int process_measurement(struct file *file, int mask, int function, 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 @@ static int process_measurement(struct file *file, int mask, int function, 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 @@ static int process_measurement(struct file *file, int mask, int function, 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 @@ static int process_measurement(struct file *file, int mask, int function, 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 @@ static int process_measurement(struct file *file, int mask, int function, 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 5efe2ecc538d3..37e48e9633db1 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 @@ 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 atomic_flags; enum integrity_status ima_file_status:4; enum integrity_status ima_mmap_status:4; enum integrity_status ima_bprm_status:4; |