Hi, Could I ask FS maintainers to test IMA with this patch additionally and provide ack/tested. We tested but may be you have and some special testing. Thanks in advance, Dmitry On Tue, Dec 5, 2017 at 9:06 PM, Dmitry Kasatkin <dmitry.kasatkin@xxxxxxxxx> wrote: > The original design was discussed 3+ years ago, but was never completed/upstreamed. > Based on the recent discussions with Linus > https://patchwork.kernel.org/patch/9975919, I've rebased this patch. > > 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. > > More recently some filesystems have replaced their filesystem specific > lock with the global i_rwsem to read a file. As a result, when IMA > attempts to calculate the file hash, reading the file attempts to take > the i_rwsem again. > > 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_rwsem. > > Original flags were cleared in chmod(), setxattr() or removwxattr() hooks > and tested when file was closed or opened again. New atomic flags are set > or cleared in those hooks and tested to clear iint->flags on close or on open. > > Atomic flags are following: > * IMA_CHANGE_ATTR - indicates that chATTR() was called (chmod, chown, chgrp) > and file attributes have changed. On file open, it causes IMA to clear > iint->flags to re-evaluate policy and perform IMA functions again. > * IMA_CHANGE_XATTR - indicates that setxattr or removexattr was called and > extended attributes have changed. On file open, it causes IMA to clear > iint->flags IMA_DONE_MASK to re-appraise. > * IMA_UPDATE_XATTR - indicates that security.ima needs to be updated. > It is cleared if file policy changes and no update is needed. > * IMA_DIGSIG - indicates that file security.ima has signature and file > security.ima must not update to file has on file close. > * IMA_MUST_MEASURE - indicates the file is in the measurement policy. > > Changes in v6: > * introduce the atomic flag IMA_MUST_MEASURE to indicate that a file is in > the measurement policy. It is used instead of the IMA_MEASURE (iint->flags) > to detect ToMToU violation and when iint->mutex is unlocked and behind inode > lock only (same as some other flags). Issue reported by Roberto Sassu. > > Changes in v5: > * use of inode_lock() and inode_unlock() > > 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@xxxxxxxxxx> > --- > security/integrity/iint.c | 2 + > security/integrity/ima/ima_appraise.c | 27 +++++++------- > security/integrity/ima/ima_main.c | 70 ++++++++++++++++++++++++----------- > security/integrity/integrity.h | 18 ++++++--- > 4 files changed, 77 insertions(+), 40 deletions(-) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index c84e058..d726ba23 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -155,12 +155,14 @@ 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_read_status = INTEGRITY_UNKNOWN; > iint->evm_status = INTEGRITY_UNKNOWN; > iint->measured_pcrs = 0; > + 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 9a54c77..3fc96dbd 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -251,6 +251,7 @@ int ima_appraise_measurement(enum ima_hooks func, > 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 > @@ -269,7 +270,7 @@ int ima_appraise_measurement(enum ima_hooks func, > 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, > @@ -320,14 +321,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, 0, ima_hash_algo); > if (rc < 0) > return; > > + inode_lock(file_inode(file)); > ima_fix_xattr(dentry, iint); > + inode_unlock(file_inode(file)); > } > > /** > @@ -350,16 +353,14 @@ void ima_inode_post_setattr(struct dentry *dentry) > return; > > must_appraise = ima_must_appraise(inode, MAY_ACCESS, POST_SETATTR); > + if (!must_appraise) > + __vfs_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_RULE_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) > - __vfs_removexattr(dentry, XATTR_NAME_IMA); > } > > /* > @@ -388,12 +389,12 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig) > iint = integrity_iint_find(inode); > if (!iint) > return; > - > - iint->flags &= ~IMA_DONE_MASK; > iint->measured_pcrs = 0; > + 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 7706546..edf4e07 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -96,10 +96,13 @@ static void ima_rdwr_violation_check(struct file *file, > if (!iint) > iint = integrity_iint_find(inode); > /* IMA_MEASURE is set from reader side */ > - if (iint && (iint->flags & IMA_MEASURE)) > + if (iint && test_bit(IMA_MUST_MEASURE, > + &iint->atomic_flags)) > send_tomtou = true; > } > } else { > + if (must_measure) > + set_bit(IMA_MUST_MEASURE, &iint->atomic_flags); > if ((atomic_read(&inode->i_writecount) > 0) && must_measure) > send_writers = true; > } > @@ -121,21 +124,24 @@ 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; > > - inode_lock(inode); > + 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); > iint->measured_pcrs = 0; > - if (iint->flags & IMA_APPRAISE) > + if (update) > ima_update_xattr(iint, file); > } > } > - inode_unlock(inode); > + mutex_unlock(&iint->mutex); > } > > /** > @@ -168,7 +174,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > char *pathbuf = NULL; > char filename[NAME_MAX]; > const char *pathname = NULL; > - int rc = -ENOMEM, action, must_appraise; > + int rc = 0, action, must_appraise = 0; > int pcr = CONFIG_IMA_MEASURE_PCR_IDX; > struct evm_ima_xattr_data *xattr_value = NULL; > int xattr_len = 0; > @@ -199,17 +205,31 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > 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; > - } > - } > + > + inode_unlock(inode); > + > + 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, > @@ -227,7 +247,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > if (!action) { > if (must_appraise) > rc = ima_get_cache_status(iint, func); > - goto out_digsig; > + goto out_locked; > } > > template_desc = ima_template_desc_current(); > @@ -240,7 +260,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > > rc = ima_collect_measurement(iint, file, buf, size, hash_algo); > if (rc != 0 && rc != -EBADF && rc != -EINVAL) > - goto out_digsig; > + goto out_locked; > > if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */ > pathname = ima_d_path(&file->f_path, &pathbuf, filename); > @@ -248,26 +268,32 @@ static int process_measurement(struct file *file, char *buf, loff_t size, > if (action & IMA_MEASURE) > ima_store_measurement(iint, file, pathname, > xattr_value, xattr_len, pcr); > - if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) > + if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) { > + inode_lock(inode); > rc = ima_appraise_measurement(func, iint, file, pathname, > xattr_value, xattr_len, opened); > + inode_unlock(inode); > + } > if (action & IMA_AUDIT) > ima_audit_measurement(iint, pathname); > > if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO)) > rc = 0; > -out_digsig: > - if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) && > +out_locked: > + if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) && > !(iint->flags & IMA_NEW_FILE)) > rc = -EACCES; > + mutex_unlock(&iint->mutex); > kfree(xattr_value); > -out_free: > +out: > if (pathbuf) > __putname(pathbuf); > -out: > - inode_unlock(inode); > - 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 e324bf9..c64ea8f 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -29,11 +29,10 @@ > /* iint cache flags */ > #define IMA_ACTION_FLAGS 0xff000000 > #define IMA_ACTION_RULE_FLAGS 0x06000000 > -#define IMA_DIGSIG 0x01000000 > -#define IMA_DIGSIG_REQUIRED 0x02000000 > -#define IMA_PERMIT_DIRECTIO 0x04000000 > -#define IMA_NEW_FILE 0x08000000 > -#define EVM_IMMUTABLE_DIGSIG 0x10000000 > +#define IMA_DIGSIG_REQUIRED 0x01000000 > +#define IMA_PERMIT_DIRECTIO 0x02000000 > +#define IMA_NEW_FILE 0x04000000 > +#define EVM_IMMUTABLE_DIGSIG 0x08000000 > > #define IMA_DO_MASK (IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \ > IMA_APPRAISE_SUBMASK) > @@ -54,6 +53,13 @@ > #define IMA_APPRAISED_SUBMASK (IMA_FILE_APPRAISED | IMA_MMAP_APPRAISED | \ > IMA_BPRM_APPRAISED | IMA_READ_APPRAISED) > > +/* iint cache atomic_flags */ > +#define IMA_CHANGE_XATTR 0 > +#define IMA_UPDATE_XATTR 1 > +#define IMA_CHANGE_ATTR 2 > +#define IMA_DIGSIG 3 > +#define IMA_MUST_MEASURE 4 > + > enum evm_ima_xattr_type { > IMA_XATTR_DIGEST = 0x01, > EVM_XATTR_HMAC, > @@ -102,10 +108,12 @@ 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 measured_pcrs; > + 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; > -- > 2.7.4 > -- Thanks, Dmitry
