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);
