Re: [PATCHv6 1/1] ima: re-introduce own integrity cache lock

[Darrick J. Wong]

[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