Re: [PATCH] integrity: make 'sync' update the inode integrity state

[Mimi Zohar <zohar@linux.ibm.com>]

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