[might as well cc linux-xfs] On Thu, Dec 14, 2017 at 12:22:37AM +0200, Dmitry Kasatkin wrote: > 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. Super-late to this party, but unless xfstests has automated tests to set up IMA on top of an existing filesystem then I most likely have no idea /how/ to test IMA. I did a quick grep of xfstests git and I don't see anything IMA-related. --D > > 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
