Re: [PATCH 3/3] ima: update the file measurement on writes

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

On Mon, 2019-09-02 at 12:45 +0300, Janne Karhunen wrote:
> From: Konsta Karsisto <konsta.karsisto@gmail.com>
> 
> Hook do_writepages() in order to do IMA measurement of an inode
> that is written out.

With this explanation I would expect to see a security/ima hook in
do_writepages(), nothing else.  There's a lot more going on here than
that.  The memory management maintainer(s) doesn't really care about
IMA.  Please break this patch up so that it is easier to review and
upstream.

My comments on the first patch in this patch set (e.g. function names,
ifdefs in C code, workqueue belonging in a separate patch) are also
applicable to this patch.

> 
> Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'
> 
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---

[snip]


> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 1804f64ff43c..d5041c625529 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -38,6 +38,7 @@
>  #include <linux/sched/rt.h>
>  #include <linux/sched/signal.h>
>  #include <linux/mm_inline.h>
> +#include <linux/ima.h>
>  #include <trace/events/writeback.h>
>  
>  #include "internal.h"
> @@ -2347,6 +2348,12 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  		cond_resched();
>  		congestion_wait(BLK_RW_ASYNC, HZ/50);
>  	}
> +	if (ret == 0) {
> +		if (wbc->sync_mode == WB_SYNC_ALL)
> +			ima_inode_update(mapping->host);
> +		else
> +			ima_inode_delayed_update(mapping->host);

It's hard enough upstreaming a single security or IMA hook.  There's
no need for two.  security/IMA hooks are normally based on function
names (eg. ima_do_writepages).  The sync_mode should be an argument.

> +	}
>  	return ret;
>  }
>  


> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 46d28cdb6466..affc74a07125 100644

> @@ -425,6 +430,42 @@ int ima_bprm_check(struct linux_binprm *bprm)
>  }
>  
>  #ifdef CONFIG_IMA_MEASURE_WRITES
> +void ima_get_file(struct integrity_iint_cache *iint,
> +		  struct file *file)
> +{
> +	struct ima_fl_entry *e;
> +
> +	if (!iint || !file)
> +		return;
> +	if (!(file->f_mode & FMODE_WRITE) ||
> +	    !test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> +		return;
> +
> +	list_for_each_entry(e, &iint->file_list, list) {
> +		if (e->file == file)
> +			return;
> +	}
> +	e = kmalloc(sizeof(*e), GFP_KERNEL);
> +	if (!e)
> +		return;
> +	e->file = file;
> +	list_add(&e->list, &iint->file_list);
> +}
> +
> +void ima_put_file(struct integrity_iint_cache *iint,
> +		  struct file *file)
> +{
> +	struct ima_fl_entry *e;
> +
> +	list_for_each_entry(e, &iint->file_list, list) {
> +		if (e->file == file) {
> +			list_del(&e->list);
> +			kfree(e);
> +			break;
> +		}
> +	}
> +}
> +

Functions with "get" or "put" in their name increment/decrement a
reference count.  No reference count is being updated here.

Mimi