Hi Mimi,
I will take a couple of days to test and think this through a bit
better, there are extra cases like file creation without ever writing
to it (also ending up in appraisal failure for no reason) and msync()
that would also be beneficial to tackle on the same go.
--
Janne
On Thu, Apr 11, 2019 at 4:03 PM Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
>
> Hi Janne,
>
> I need to finish up a couple of other things before vacation. Below
> are just a few comments/questions for you to think about.
>
> On Wed, 2019-04-10 at 17:56 +0300, Janne Karhunen wrote:
>
> > +/**
> > + * ima_file_update - called from sync to update xattrs
> > + * @file: pointer to file structure being updated
> > + */
> > +void ima_file_update(struct file *file)
> > +{
> > + struct inode *inode = file_inode(file);
> > + struct integrity_iint_cache *iint;
> > +
> > + if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > + return;
> > +
> > + iint = integrity_iint_find(inode);
> > + if (!iint)
> > + return;
> > +
> > + mutex_lock(&iint->mutex);
> > + if (atomic_read(&inode->i_writecount) == 1) {
>
> This test limits the number of opened writers. Only if there is a
> single writer opened, will the xattr be updated. Is this what you
> intended?
>
> Your testing should open the same file for write multiple times.
>
> > + clear_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
> > + if (!IS_I_VERSION(inode) ||
> > + !inode_eq_iversion(inode, iint->version)) {
> > + iint->flags &= ~IMA_COLLECTED;
> > + ima_update_xattr(iint, file);
>
> Relatively recently there were some changes to iversion so that it
> isn't being updated as frequently. Can we use i_version here?
>
> > + }
> > + }
> > + mutex_unlock(&iint->mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(ima_file_update);
> > +
> > /**
> > * ima_path_check - based on policy, collect/store measurement.
> > * @file: pointer to the file to be measured
> > diff --git a/security/security.c b/security/security.c
> > index 23cbb1a295a3..6a0980a1df22 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1451,6 +1451,11 @@ int security_file_open(struct file *file)
> > return fsnotify_perm(file, MAY_OPEN);
> > }
> >
> > +void security_file_sync(struct file *file)
> > +{
> > + ima_file_update(file);
> > +}
> > +
>
> Either this is an LSM hook or it isn't. If it's an LSM hook it needs
> to be similar to the existing hooks. If it's an IMA hook, like
> ima_file_check() or ima_file_free(), then call it directly.
>
> Normally the function name is related to the LSM hook name. For
> example, I would name it ima_file_sync.
>
> Mimi
>
> > int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
> > {
> > int rc = lsm_task_alloc(task);
>
