Hi,
Finally. Now I think I'm almost happy with the overall construction
and my gut feeling is telling me I can make the hashes reflect the
filesystem state pretty well, maybe even over 99.9% of the time given
the workload stock android is generating.
The pieces that we did:
- initialize hashes correctly on open()
- hook 'sync'
- hook 'msync'
- hook 'truncate'
- introduce a per-cpu workqueue that gets work items from write and
dirty page flush. Long-running write is allowed to go as is (no
performance penalty from re-measurements), but once the the write goes
silent workqueue item is flushed and the file is hashed.
Before I uplift this construction against the mainline, any thoughts
about this? All I can say so far is that it runs and it seems to keep
the system relatively crash tolerant. 'Don't let the perfect be the
enemy of the better' I guess...
--
Janne
On Fri, Apr 12, 2019 at 3:40 PM Janne Karhunen <janne.karhunen@xxxxxxxxx> wrote:
>
> 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);
> >
