Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Mimi Zohar <zohar@linux.ibm.com>
To: Janne Karhunen <janne.karhunen@gmail.com>,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, linux-mm@kvack.org,
	viro@zeniv.linux.org.uk
Cc: Konsta Karsisto <konsta.karsisto@gmail.com>
Subject: Re: [PATCH 3/3] ima: update the file measurement on writes
Date: Sun, 08 Sep 2019 13:07:42 -0400
Message-ID: <1567962462.4614.202.camel@linux.ibm.com> (raw)
In-Reply-To: <20190902094540.12786-3-janne.karhunen@gmail.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


  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-02  9:45 [PATCH 1/3] ima: keep the integrity state of open files up to date Janne Karhunen
2019-09-02  9:45 ` [PATCH 2/3] ima: update the file measurement on truncate Janne Karhunen
2019-09-08 15:38   ` Mimi Zohar
2019-09-02  9:45 ` [PATCH 3/3] ima: update the file measurement on writes Janne Karhunen
2019-09-08 17:07   ` Mimi Zohar [this message]
2019-09-02 11:31 ` [PATCH 1/3] ima: keep the integrity state of open files up to date kbuild test robot
2019-09-02 12:57 ` kbuild test robot
2019-09-08 16:35 ` Mimi Zohar
2019-09-09 21:39 ` Eric Biggers
2019-09-10  7:04   ` Janne Karhunen
2019-09-15 20:24     ` Eric Biggers
2019-09-16 11:45       ` Janne Karhunen
2019-09-17  4:23         ` Eric Biggers
2019-09-17  7:24           ` Janne Karhunen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1567962462.4614.202.camel@linux.ibm.com \
    --to=zohar@linux.ibm.com \
    --cc=janne.karhunen@gmail.com \
    --cc=konsta.karsisto@gmail.com \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git