From 8549164143a5431f9d9ea846acaa35a862410d9c Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:41:18 -0400
Subject: IMA: use rbtree instead of radix tree for inode information cache

The IMA code needs to store the number of tasks which have an open fd
granting permission to write a file even when IMA is not in use.  It
needs this information in order to be enabled at a later point in time
without losing it's integrity garantees.

At the moment that means we store a little bit of data about every inode
in a cache.  We use a radix tree key'd on the inode's memory address.
Dave Chinner pointed out that a radix tree is a terrible data structure
for such a sparse key space.  This patch switches to using an rbtree
which should be more efficient.

Bug report from Dave:

 "I just noticed that slabtop was reporting an awfully high usage of
  radix tree nodes:

   OBJS ACTIVE  USE OBJ SIZE  SLABS OBJ/SLAB CACHE SIZE NAME
  4200331 2778082  66%    0.55K 144839       29   2317424K radix_tree_node
  2321500 2060290  88%    1.00K  72581       32   2322592K xfs_inode
  2235648 2069791  92%    0.12K  69864       32    279456K iint_cache

  That is, 2.7M radix tree nodes are allocated, and the cache itself is
  consuming 2.3GB of RAM.  I know that the XFS inodei caches are indexed
  by radix tree node, but for 2 million cached inodes that would mean a
  density of 1 inode per radix tree node, which for a system with 16M
  inodes in the filsystems is an impossibly low density.  The worst I've
  seen in a production system like kernel.org is about 20-25% density,
  which would mean about 150-200k radix tree nodes for that many inodes.
  So it's not the inode cache.

  So I looked up what the iint_cache was.  It appears to used for
  storing per-inode IMA information, and uses a radix tree for indexing.
  It uses the *address* of the struct inode as the indexing key.  That
  means the key space is extremely sparse - for XFS the struct inode
  addresses are approximately 1000 bytes apart, which means the closest
  the radix tree index keys get is ~1000.  Which means that there is a
  single entry per radix tree leaf node, so the radix tree is using
  roughly 550 bytes for every 120byte structure being cached.  For the
  above example, it's probably wasting close to 1GB of RAM...."

Reported-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima.h      |   6 +--
 security/integrity/ima/ima_iint.c | 105 ++++++++++++++++++++++++++------------
 2 files changed, 75 insertions(+), 36 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3fbcd1dda0e..7557791e954 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -100,6 +100,8 @@ static inline unsigned long ima_hash_key(u8 *digest)
 
 /* integrity data associated with an inode */
 struct ima_iint_cache {
+	struct rb_node rb_node; /* rooted in ima_iint_tree */
+	struct inode *inode;	/* back pointer to inode in question */
 	u64 version;		/* track inode changes */
 	unsigned long flags;
 	u8 digest[IMA_DIGEST_SIZE];
@@ -108,7 +110,6 @@ struct ima_iint_cache {
 	long writecount;	/* measured files writecount */
 	long opencount;		/* opens reference count */
 	struct kref refcount;	/* ima_iint_cache reference count */
-	struct rcu_head rcu;
 };
 
 /* LIM API function definitions */
@@ -122,13 +123,12 @@ int ima_store_template(struct ima_template_entry *entry, int violation,
 void ima_template_show(struct seq_file *m, void *e,
 		       enum ima_show_type show);
 
-/* radix tree calls to lookup, insert, delete
+/* rbtree tree calls to lookup, insert, delete
  * integrity data associated with an inode.
  */
 struct ima_iint_cache *ima_iint_insert(struct inode *inode);
 struct ima_iint_cache *ima_iint_find_get(struct inode *inode);
 void iint_free(struct kref *kref);
-void iint_rcu_free(struct rcu_head *rcu);
 
 /* IMA policy related functions */
 enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index afba4aef812..8395f0f5e9b 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -12,21 +12,48 @@
  * File: ima_iint.c
  * 	- implements the IMA hooks: ima_inode_alloc, ima_inode_free
  *	- cache integrity information associated with an inode
- *	  using a radix tree.
+ *	  using a rbtree tree.
  */
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
-#include <linux/radix-tree.h>
+#include <linux/rbtree.h>
 #include "ima.h"
 
-RADIX_TREE(ima_iint_store, GFP_ATOMIC);
-DEFINE_SPINLOCK(ima_iint_lock);
+static struct rb_root ima_iint_tree = RB_ROOT;
+static DEFINE_SPINLOCK(ima_iint_lock);
 static struct kmem_cache *iint_cache __read_mostly;
 
 int iint_initialized = 0;
 
-/* ima_iint_find_get - return the iint associated with an inode
+/*
+ * __ima_iint_find - return the iint associated with an inode
+ */
+static struct ima_iint_cache *__ima_iint_find(struct inode *inode)
+{
+	struct ima_iint_cache *iint;
+	struct rb_node *n = ima_iint_tree.rb_node;
+
+	assert_spin_locked(&ima_iint_lock);
+
+	while (n) {
+		iint = rb_entry(n, struct ima_iint_cache, rb_node);
+
+		if (inode < iint->inode)
+			n = n->rb_left;
+		else if (inode > iint->inode)
+			n = n->rb_right;
+		else
+			break;
+	}
+	if (!n)
+		return NULL;
+
+	return iint;
+}
+
+/*
+ * ima_iint_find_get - return the iint associated with an inode
  *
  * ima_iint_find_get gets a reference to the iint. Caller must
  * remember to put the iint reference.
@@ -35,13 +62,12 @@ struct ima_iint_cache *ima_iint_find_get(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	rcu_read_lock();
-	iint = radix_tree_lookup(&ima_iint_store, (unsigned long)inode);
-	if (!iint)
-		goto out;
-	kref_get(&iint->refcount);
-out:
-	rcu_read_unlock();
+	spin_lock(&ima_iint_lock);
+	iint = __ima_iint_find(inode);
+	if (iint)
+		kref_get(&iint->refcount);
+	spin_unlock(&ima_iint_lock);
+
 	return iint;
 }
 
@@ -51,25 +77,43 @@ out:
  */
 int ima_inode_alloc(struct inode *inode)
 {
-	struct ima_iint_cache *iint = NULL;
-	int rc = 0;
+	struct rb_node **p;
+	struct rb_node *new_node, *parent = NULL;
+	struct ima_iint_cache *new_iint, *test_iint;
+	int rc;
 
-	iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
-	if (!iint)
+	new_iint = kmem_cache_alloc(iint_cache, GFP_NOFS);
+	if (!new_iint)
 		return -ENOMEM;
 
-	rc = radix_tree_preload(GFP_NOFS);
-	if (rc < 0)
-		goto out;
+	new_iint->inode = inode;
+	new_node = &new_iint->rb_node;
 
 	spin_lock(&ima_iint_lock);
-	rc = radix_tree_insert(&ima_iint_store, (unsigned long)inode, iint);
+
+	p = &ima_iint_tree.rb_node;
+	while (*p) {
+		parent = *p;
+		test_iint = rb_entry(parent, struct ima_iint_cache, rb_node);
+
+		rc = -EEXIST;
+		if (inode < test_iint->inode)
+			p = &(*p)->rb_left;
+		else if (inode > test_iint->inode)
+			p = &(*p)->rb_right;
+		else
+			goto out_err;
+	}
+
+	rb_link_node(new_node, parent, p);
+	rb_insert_color(new_node, &ima_iint_tree);
+
 	spin_unlock(&ima_iint_lock);
-	radix_tree_preload_end();
-out:
-	if (rc < 0)
-		kmem_cache_free(iint_cache, iint);
 
+	return 0;
+out_err:
+	spin_unlock(&ima_iint_lock);
+	kref_put(&new_iint->refcount, iint_free);
 	return rc;
 }
 
@@ -99,13 +143,6 @@ void iint_free(struct kref *kref)
 	kmem_cache_free(iint_cache, iint);
 }
 
-void iint_rcu_free(struct rcu_head *rcu_head)
-{
-	struct ima_iint_cache *iint = container_of(rcu_head,
-						   struct ima_iint_cache, rcu);
-	kref_put(&iint->refcount, iint_free);
-}
-
 /**
  * ima_inode_free - called on security_inode_free
  * @inode: pointer to the inode
@@ -117,10 +154,12 @@ void ima_inode_free(struct inode *inode)
 	struct ima_iint_cache *iint;
 
 	spin_lock(&ima_iint_lock);
-	iint = radix_tree_delete(&ima_iint_store, (unsigned long)inode);
+	iint = __ima_iint_find(inode);
+	if (iint)
+		rb_erase(&iint->rb_node, &ima_iint_tree);
 	spin_unlock(&ima_iint_lock);
 	if (iint)
-		call_rcu(&iint->rcu, iint_rcu_free);
+		kref_put(&iint->refcount, iint_free);
 }
 
 static void init_once(void *foo)
-- 
cgit v1.2.3-70-g09d2


From b575156dafef208415ff0842c392733d16d4ccf1 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:41:26 -0400
Subject: IMA: drop the inode opencount since it isn't needed for operation

The opencount was used to help debugging to make sure that everything
which created a struct file also correctly made the IMA calls.  Since we
moved all of that into the VFS this isn't as necessary.  We should be
able to get the same amount of debugging out of just the reader and
write count.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima.h      |  1 -
 security/integrity/ima/ima_iint.c |  6 ------
 security/integrity/ima/ima_main.c | 10 +++-------
 3 files changed, 3 insertions(+), 14 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7557791e954..3d701084eac 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -108,7 +108,6 @@ struct ima_iint_cache {
 	struct mutex mutex;	/* protects: version, flags, digest */
 	long readcount;		/* measured files readcount */
 	long writecount;	/* measured files writecount */
-	long opencount;		/* opens reference count */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 8395f0f5e9b..8e64313ed18 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -134,11 +134,6 @@ void iint_free(struct kref *kref)
 		       iint->writecount);
 		iint->writecount = 0;
 	}
-	if (iint->opencount != 0) {
-		printk(KERN_INFO "%s: opencount: %ld\n", __func__,
-		       iint->opencount);
-		iint->opencount = 0;
-	}
 	kref_init(&iint->refcount);
 	kmem_cache_free(iint_cache, iint);
 }
@@ -172,7 +167,6 @@ static void init_once(void *foo)
 	mutex_init(&iint->mutex);
 	iint->readcount = 0;
 	iint->writecount = 0;
-	iint->opencount = 0;
 	kref_init(&iint->refcount);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e662b89d407..995bd1b98fa 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -122,7 +122,6 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 {
 	BUG_ON(!mutex_is_locked(&iint->mutex));
 
-	iint->opencount++;
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount++;
 	if (mode & FMODE_WRITE)
@@ -181,7 +180,6 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 	mode_t mode = file->f_mode;
 	BUG_ON(!mutex_is_locked(&iint->mutex));
 
-	iint->opencount--;
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount--;
 	if (mode & FMODE_WRITE) {
@@ -192,13 +190,11 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 		}
 	}
 
-	if (((iint->opencount < 0) ||
-	     (iint->readcount < 0) ||
+	if (((iint->readcount < 0) ||
 	     (iint->writecount < 0)) &&
 	    !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld o:%ld)\n",
-		       __func__, iint->readcount, iint->writecount,
-		       iint->opencount);
+		printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld)\n",
+		       __func__, iint->readcount, iint->writecount);
 		dump_stack();
 	}
 }
-- 
cgit v1.2.3-70-g09d2


From 497f32337073a2da102c49a53779097b5394711b Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:41:32 -0400
Subject: IMA: use unsigned int instead of long for counters

Currently IMA uses 2 longs in struct inode.  To save space (and as it
seems impossible to overflow 32 bits) we switch these to unsigned int.
The switch to unsigned does require slightly different checks for
underflow, but it isn't complex.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima.h      |  4 ++--
 security/integrity/ima/ima_iint.c |  4 ++--
 security/integrity/ima/ima_main.c | 15 ++++++++++-----
 3 files changed, 14 insertions(+), 9 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3d701084eac..000d13ab1a2 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -106,8 +106,8 @@ struct ima_iint_cache {
 	unsigned long flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
-	long readcount;		/* measured files readcount */
-	long writecount;	/* measured files writecount */
+	unsigned int readcount;	/* measured files readcount */
+	unsigned int writecount;/* measured files writecount */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 8e64313ed18..db71a13f27f 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -125,12 +125,12 @@ void iint_free(struct kref *kref)
 	iint->version = 0;
 	iint->flags = 0UL;
 	if (iint->readcount != 0) {
-		printk(KERN_INFO "%s: readcount: %ld\n", __func__,
+		printk(KERN_INFO "%s: readcount: %u\n", __func__,
 		       iint->readcount);
 		iint->readcount = 0;
 	}
 	if (iint->writecount != 0) {
-		printk(KERN_INFO "%s: writecount: %ld\n", __func__,
+		printk(KERN_INFO "%s: writecount: %u\n", __func__,
 		       iint->writecount);
 		iint->writecount = 0;
 	}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 995bd1b98fa..5a1bf3df11f 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -178,11 +178,18 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 			   struct file *file)
 {
 	mode_t mode = file->f_mode;
+	bool dump = false;
+
 	BUG_ON(!mutex_is_locked(&iint->mutex));
 
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
+		if (unlikely(iint->readcount == 0))
+			dump = true;
 		iint->readcount--;
+	}
 	if (mode & FMODE_WRITE) {
+		if (unlikely(iint->writecount == 0))
+			dump = true;
 		iint->writecount--;
 		if (iint->writecount == 0) {
 			if (iint->version != inode->i_version)
@@ -190,10 +197,8 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 		}
 	}
 
-	if (((iint->readcount < 0) ||
-	     (iint->writecount < 0)) &&
-	    !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%ld w:%ld)\n",
+	if (dump && !ima_limit_imbalance(file)) {
+		printk(KERN_INFO "%s: open/free imbalance (r:%u w:%u)\n",
 		       __func__, iint->readcount, iint->writecount);
 		dump_stack();
 	}
-- 
cgit v1.2.3-70-g09d2


From 15aac676778f206b42c4d7782b08f89246680485 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:41:39 -0400
Subject: IMA: convert internal flags from long to char

The IMA flags is an unsigned long but there is only 1 flag defined.
Lets save a little space and make it a char.  This packs nicely next to
the array of u8's.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 000d13ab1a2..f7af0110bcd 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -96,14 +96,14 @@ static inline unsigned long ima_hash_key(u8 *digest)
 }
 
 /* iint cache flags */
-#define IMA_MEASURED		1
+#define IMA_MEASURED		0x01
 
 /* integrity data associated with an inode */
 struct ima_iint_cache {
 	struct rb_node rb_node; /* rooted in ima_iint_tree */
 	struct inode *inode;	/* back pointer to inode in question */
 	u64 version;		/* track inode changes */
-	unsigned long flags;
+	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
 	unsigned int readcount;	/* measured files readcount */
-- 
cgit v1.2.3-70-g09d2


From ad16ad00c34d3f320a5876b3d711ef6bc81362e1 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:41:45 -0400
Subject: IMA: use inode->i_lock to protect read and write counters

Currently IMA used the iint->mutex to protect the i_readcount and
i_writecount.  This patch uses the inode->i_lock since we are going to
start using in inode objects and that is the most appropriate lock.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima.h      |  1 +
 security/integrity/ima/ima_main.c | 57 ++++++++++++++++-----------------------
 2 files changed, 24 insertions(+), 34 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f7af0110bcd..80aca3d2cb7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -106,6 +106,7 @@ struct ima_iint_cache {
 	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
+	/* protected by inode->i_lock */
 	unsigned int readcount;	/* measured files readcount */
 	unsigned int writecount;/* measured files writecount */
 	struct kref refcount;	/* ima_iint_cache reference count */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5a1bf3df11f..2f9b5d50424 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -85,42 +85,12 @@ out:
 	return found;
 }
 
-/* ima_read_write_check - reflect possible reading/writing errors in the PCR.
- *
- * When opening a file for read, if the file is already open for write,
- * the file could change, resulting in a file measurement error.
- *
- * Opening a file for write, if the file is already open for read, results
- * in a time of measure, time of use (ToMToU) error.
- *
- * In either case invalidate the PCR.
- */
-enum iint_pcr_error { TOMTOU, OPEN_WRITERS };
-static void ima_read_write_check(enum iint_pcr_error error,
-				 struct ima_iint_cache *iint,
-				 struct inode *inode,
-				 const unsigned char *filename)
-{
-	switch (error) {
-	case TOMTOU:
-		if (iint->readcount > 0)
-			ima_add_violation(inode, filename, "invalid_pcr",
-					  "ToMToU");
-		break;
-	case OPEN_WRITERS:
-		if (iint->writecount > 0)
-			ima_add_violation(inode, filename, "invalid_pcr",
-					  "open_writers");
-		break;
-	}
-}
-
 /*
  * Update the counts given an fmode_t
  */
 static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 {
-	BUG_ON(!mutex_is_locked(&iint->mutex));
+	assert_spin_locked(&iint->inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount++;
@@ -146,6 +116,7 @@ void ima_counts_get(struct file *file)
 	fmode_t mode = file->f_mode;
 	struct ima_iint_cache *iint;
 	int rc;
+	bool send_tomtou = false, send_writers = false;
 
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
@@ -153,22 +124,35 @@ void ima_counts_get(struct file *file)
 	if (!iint)
 		return;
 	mutex_lock(&iint->mutex);
+	spin_lock(&inode->i_lock);
+
 	if (!ima_initialized)
 		goto out;
+
 	rc = ima_must_measure(iint, inode, MAY_READ, FILE_CHECK);
 	if (rc < 0)
 		goto out;
 
 	if (mode & FMODE_WRITE) {
-		ima_read_write_check(TOMTOU, iint, inode, dentry->d_name.name);
+		if (iint->readcount)
+			send_tomtou = true;
 		goto out;
 	}
-	ima_read_write_check(OPEN_WRITERS, iint, inode, dentry->d_name.name);
+
+	if (atomic_read(&inode->i_writecount) > 0)
+		send_writers = true;
 out:
 	ima_inc_counts(iint, file->f_mode);
+	spin_unlock(&inode->i_lock);
 	mutex_unlock(&iint->mutex);
-
 	kref_put(&iint->refcount, iint_free);
+
+	if (send_tomtou)
+		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
+				  "ToMToU");
+	if (send_writers)
+		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
+				  "open_writers");
 }
 
 /*
@@ -181,6 +165,7 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 	bool dump = false;
 
 	BUG_ON(!mutex_is_locked(&iint->mutex));
+	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
 		if (unlikely(iint->readcount == 0))
@@ -223,7 +208,11 @@ void ima_file_free(struct file *file)
 		return;
 
 	mutex_lock(&iint->mutex);
+	spin_lock(&inode->i_lock);
+
 	ima_dec_counts(iint, inode, file);
+
+	spin_unlock(&inode->i_lock);
 	mutex_unlock(&iint->mutex);
 	kref_put(&iint->refcount, iint_free);
 }
-- 
cgit v1.2.3-70-g09d2


From b9593d309d17c57e9ddc3934d641902533896ca9 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:41:52 -0400
Subject: IMA: use i_writecount rather than a private counter

IMA tracks the number of struct files which are holding a given inode
readonly and the number which are holding the inode write or r/w.  It
needs this information so when a new reader or writer comes in it can
tell if this new file will be able to invalidate results it already made
about existing files.

aka if a task is holding a struct file open RO, IMA measured the file
and recorded those measurements and then a task opens the file RW IMA
needs to note in the logs that the old measurement may not be correct.
It's called a "Time of Measure Time of Use" (ToMToU) issue.  The same is
true is a RO file is opened to an inode which has an open writer.  We
cannot, with any validity, measure the file in question since it could
be changing.

This patch attempts to use the i_writecount field to track writers.  The
i_writecount field actually embeds more information in it's value than
IMA needs but it should work for our purposes and allow us to shrink the
struct inode even more.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima.h      |  1 -
 security/integrity/ima/ima_iint.c |  6 ------
 security/integrity/ima/ima_main.c | 16 ++++++----------
 3 files changed, 6 insertions(+), 17 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 80aca3d2cb7..b546b90f5fa 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -108,7 +108,6 @@ struct ima_iint_cache {
 	struct mutex mutex;	/* protects: version, flags, digest */
 	/* protected by inode->i_lock */
 	unsigned int readcount;	/* measured files readcount */
-	unsigned int writecount;/* measured files writecount */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index db71a13f27f..e68891f8d55 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -129,11 +129,6 @@ void iint_free(struct kref *kref)
 		       iint->readcount);
 		iint->readcount = 0;
 	}
-	if (iint->writecount != 0) {
-		printk(KERN_INFO "%s: writecount: %u\n", __func__,
-		       iint->writecount);
-		iint->writecount = 0;
-	}
 	kref_init(&iint->refcount);
 	kmem_cache_free(iint_cache, iint);
 }
@@ -166,7 +161,6 @@ static void init_once(void *foo)
 	iint->flags = 0UL;
 	mutex_init(&iint->mutex);
 	iint->readcount = 0;
-	iint->writecount = 0;
 	kref_init(&iint->refcount);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2f9b5d50424..24660bf3f82 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -94,8 +94,6 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		iint->readcount++;
-	if (mode & FMODE_WRITE)
-		iint->writecount++;
 }
 
 /*
@@ -173,18 +171,16 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 		iint->readcount--;
 	}
 	if (mode & FMODE_WRITE) {
-		if (unlikely(iint->writecount == 0))
+		if (atomic_read(&inode->i_writecount) <= 0)
 			dump = true;
-		iint->writecount--;
-		if (iint->writecount == 0) {
-			if (iint->version != inode->i_version)
-				iint->flags &= ~IMA_MEASURED;
-		}
+		if (atomic_read(&inode->i_writecount) == 1 &&
+		    iint->version != inode->i_version)
+			iint->flags &= ~IMA_MEASURED;
 	}
 
 	if (dump && !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%u w:%u)\n",
-		       __func__, iint->readcount, iint->writecount);
+		printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
+		       __func__, iint->readcount);
 		dump_stack();
 	}
 }
-- 
cgit v1.2.3-70-g09d2


From a178d2027d3198b0a04517d764326ab71cd73da2 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:41:59 -0400
Subject: IMA: move read counter into struct inode

IMA currently allocated an inode integrity structure for every inode in
core.  This stucture is about 120 bytes long.  Most files however
(especially on a system which doesn't make use of IMA) will never need
any of this space.  The problem is that if IMA is enabled we need to
know information about the number of readers and the number of writers
for every inode on the box.  At the moment we collect that information
in the per inode iint structure and waste the rest of the space.  This
patch moves those counters into the struct inode so we can eventually
stop allocating an IMA integrity structure except when absolutely
needed.

This patch does the minimum needed to move the location of the data.
Further cleanups, especially the location of counter updates, may still
be possible.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/inode.c                        |  1 +
 include/linux/fs.h                |  4 ++++
 security/integrity/ima/ima.h      |  3 +--
 security/integrity/ima/ima_api.c  |  2 +-
 security/integrity/ima/ima_iint.c | 11 +++++------
 security/integrity/ima/ima_main.c | 35 ++++++++++-------------------------
 6 files changed, 22 insertions(+), 34 deletions(-)

(limited to 'security')

diff --git a/fs/inode.c b/fs/inode.c
index 86464332e59..56d909d69bc 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -24,6 +24,7 @@
 #include <linux/mount.h>
 #include <linux/async.h>
 #include <linux/posix_acl.h>
+#include <linux/ima.h>
 
 /*
  * This is needed for the following functions:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 63d069bd80b..01e3a0047fe 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -776,6 +776,10 @@ struct inode {
 
 	unsigned int		i_flags;
 
+#ifdef CONFIG_IMA
+	/* protected by i_lock */
+	unsigned int		i_readcount; /* struct files open RO */
+#endif
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
 	void			*i_security;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b546b90f5fa..27849e1656d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -70,6 +70,7 @@ int ima_init(void);
 void ima_cleanup(void);
 int ima_fs_init(void);
 void ima_fs_cleanup(void);
+int ima_inode_alloc(struct inode *inode);
 int ima_add_template_entry(struct ima_template_entry *entry, int violation,
 			   const char *op, struct inode *inode);
 int ima_calc_hash(struct file *file, char *digest);
@@ -106,8 +107,6 @@ struct ima_iint_cache {
 	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
-	/* protected by inode->i_lock */
-	unsigned int readcount;	/* measured files readcount */
 	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 52015d098fd..d3963de6003 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -116,7 +116,7 @@ int ima_must_measure(struct ima_iint_cache *iint, struct inode *inode,
 {
 	int must_measure;
 
-	if (iint->flags & IMA_MEASURED)
+	if (iint && iint->flags & IMA_MEASURED)
 		return 1;
 
 	must_measure = ima_match_policy(inode, function, mask);
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index e68891f8d55..0936a7197e4 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -124,11 +124,6 @@ void iint_free(struct kref *kref)
 						   refcount);
 	iint->version = 0;
 	iint->flags = 0UL;
-	if (iint->readcount != 0) {
-		printk(KERN_INFO "%s: readcount: %u\n", __func__,
-		       iint->readcount);
-		iint->readcount = 0;
-	}
 	kref_init(&iint->refcount);
 	kmem_cache_free(iint_cache, iint);
 }
@@ -143,6 +138,11 @@ void ima_inode_free(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
+	if (inode->i_readcount)
+		printk(KERN_INFO "%s: readcount: %u\n", __func__, inode->i_readcount);
+
+	inode->i_readcount = 0;
+
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
 	if (iint)
@@ -160,7 +160,6 @@ static void init_once(void *foo)
 	iint->version = 0;
 	iint->flags = 0UL;
 	mutex_init(&iint->mutex);
-	iint->readcount = 0;
 	kref_init(&iint->refcount);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 24660bf3f82..2a77b14fee2 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -85,17 +85,6 @@ out:
 	return found;
 }
 
-/*
- * Update the counts given an fmode_t
- */
-static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
-{
-	assert_spin_locked(&iint->inode->i_lock);
-
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-		iint->readcount++;
-}
-
 /*
  * ima_counts_get - increment file counts
  *
@@ -112,27 +101,23 @@ void ima_counts_get(struct file *file)
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
 	fmode_t mode = file->f_mode;
-	struct ima_iint_cache *iint;
 	int rc;
 	bool send_tomtou = false, send_writers = false;
 
-	if (!iint_initialized || !S_ISREG(inode->i_mode))
+	if (!S_ISREG(inode->i_mode))
 		return;
-	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return;
-	mutex_lock(&iint->mutex);
+
 	spin_lock(&inode->i_lock);
 
 	if (!ima_initialized)
 		goto out;
 
-	rc = ima_must_measure(iint, inode, MAY_READ, FILE_CHECK);
+	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
 	if (rc < 0)
 		goto out;
 
 	if (mode & FMODE_WRITE) {
-		if (iint->readcount)
+		if (inode->i_readcount)
 			send_tomtou = true;
 		goto out;
 	}
@@ -140,10 +125,10 @@ void ima_counts_get(struct file *file)
 	if (atomic_read(&inode->i_writecount) > 0)
 		send_writers = true;
 out:
-	ima_inc_counts(iint, file->f_mode);
+	/* remember the vfs deals with i_writecount */
+	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		inode->i_readcount++;
 	spin_unlock(&inode->i_lock);
-	mutex_unlock(&iint->mutex);
-	kref_put(&iint->refcount, iint_free);
 
 	if (send_tomtou)
 		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
@@ -166,9 +151,9 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(iint->readcount == 0))
+		if (unlikely(inode->i_readcount == 0))
 			dump = true;
-		iint->readcount--;
+		inode->i_readcount--;
 	}
 	if (mode & FMODE_WRITE) {
 		if (atomic_read(&inode->i_writecount) <= 0)
@@ -180,7 +165,7 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
 
 	if (dump && !ima_limit_imbalance(file)) {
 		printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-		       __func__, iint->readcount);
+		       __func__, inode->i_readcount);
 		dump_stack();
 	}
 }
-- 
cgit v1.2.3-70-g09d2


From bc7d2a3e66b40477270c3cbe3b89b47093276e7a Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:42:05 -0400
Subject: IMA: only allocate iint when needed

IMA always allocates an integrity structure to hold information about
every inode, but only needed this structure to track the number of
readers and writers currently accessing a given inode.  Since that
information was moved into struct inode instead of the integrity struct
this patch stops allocating the integrity stucture until it is needed.
Thus greatly reducing memory usage.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima_main.c | 94 ++++++++++++++++++++++++++-------------
 security/security.c               | 10 +----
 2 files changed, 65 insertions(+), 39 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2a77b14fee2..5e3229c797f 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -141,33 +141,62 @@ out:
 /*
  * Decrement ima counts
  */
-static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
-			   struct file *file)
+static void ima_dec_counts(struct inode *inode, struct file *file)
 {
 	mode_t mode = file->f_mode;
-	bool dump = false;
 
-	BUG_ON(!mutex_is_locked(&iint->mutex));
 	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(inode->i_readcount == 0))
-			dump = true;
+		if (unlikely(inode->i_readcount == 0)) {
+			if (!ima_limit_imbalance(file)) {
+				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
+				       __func__, inode->i_readcount);
+				dump_stack();
+			}
+			return;
+		}
 		inode->i_readcount--;
 	}
-	if (mode & FMODE_WRITE) {
-		if (atomic_read(&inode->i_writecount) <= 0)
-			dump = true;
-		if (atomic_read(&inode->i_writecount) == 1 &&
-		    iint->version != inode->i_version)
-			iint->flags &= ~IMA_MEASURED;
-	}
+}
 
-	if (dump && !ima_limit_imbalance(file)) {
-		printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-		       __func__, inode->i_readcount);
-		dump_stack();
-	}
+static void ima_check_last_writer(struct ima_iint_cache *iint,
+				  struct inode *inode,
+				  struct file *file)
+{
+	mode_t mode = file->f_mode;
+
+	BUG_ON(!mutex_is_locked(&iint->mutex));
+	assert_spin_locked(&inode->i_lock);
+
+	if (mode & FMODE_WRITE &&
+	    atomic_read(&inode->i_writecount) == 1 &&
+	    iint->version != inode->i_version)
+		iint->flags &= ~IMA_MEASURED;
+}
+
+static void ima_file_free_iint(struct ima_iint_cache *iint, struct inode *inode,
+			       struct file *file)
+{
+	mutex_lock(&iint->mutex);
+	spin_lock(&inode->i_lock);
+
+	ima_dec_counts(inode, file);
+	ima_check_last_writer(iint, inode, file);
+
+	spin_unlock(&inode->i_lock);
+	mutex_unlock(&iint->mutex);
+
+	kref_put(&iint->refcount, iint_free);
+}
+
+static void ima_file_free_noiint(struct inode *inode, struct file *file)
+{
+	spin_lock(&inode->i_lock);
+
+	ima_dec_counts(inode, file);
+
+	spin_unlock(&inode->i_lock);
 }
 
 /**
@@ -175,7 +204,7 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
  * @file: pointer to file structure being freed
  *
  * Flag files that changed, based on i_version;
- * and decrement the iint readcount/writecount.
+ * and decrement the i_readcount.
  */
 void ima_file_free(struct file *file)
 {
@@ -185,17 +214,12 @@ void ima_file_free(struct file *file)
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
 	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return;
 
-	mutex_lock(&iint->mutex);
-	spin_lock(&inode->i_lock);
-
-	ima_dec_counts(iint, inode, file);
+	if (iint)
+		ima_file_free_iint(iint, inode, file);
+	else
+		ima_file_free_noiint(inode, file);
 
-	spin_unlock(&inode->i_lock);
-	mutex_unlock(&iint->mutex);
-	kref_put(&iint->refcount, iint_free);
 }
 
 static int process_measurement(struct file *file, const unsigned char *filename,
@@ -207,11 +231,21 @@ static int process_measurement(struct file *file, const unsigned char *filename,
 
 	if (!ima_initialized || !S_ISREG(inode->i_mode))
 		return 0;
+
+	rc = ima_must_measure(NULL, inode, mask, function);
+	if (rc != 0)
+		return rc;
+retry:
 	iint = ima_iint_find_get(inode);
-	if (!iint)
-		return -ENOMEM;
+	if (!iint) {
+		rc = ima_inode_alloc(inode);
+		if (!rc || rc == -EEXIST)
+			goto retry;
+		return rc;
+	}
 
 	mutex_lock(&iint->mutex);
+
 	rc = ima_must_measure(iint, inode, mask, function);
 	if (rc != 0)
 		goto out;
diff --git a/security/security.c b/security/security.c
index c53949f17d9..a3b47feccae 100644
--- a/security/security.c
+++ b/security/security.c
@@ -333,16 +333,8 @@ EXPORT_SYMBOL(security_sb_parse_opts_str);
 
 int security_inode_alloc(struct inode *inode)
 {
-	int ret;
-
 	inode->i_security = NULL;
-	ret =  security_ops->inode_alloc_security(inode);
-	if (ret)
-		return ret;
-	ret = ima_inode_alloc(inode);
-	if (ret)
-		security_inode_free(inode);
-	return ret;
+	return security_ops->inode_alloc_security(inode);
 }
 
 void security_inode_free(struct inode *inode)
-- 
cgit v1.2.3-70-g09d2


From 64c62f06bef8314a64d3189cb9c78062d54169b3 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:42:12 -0400
Subject: IMA: drop refcnt from ima_iint_cache since it isn't needed

Since finding a struct ima_iint_cache requires a valid struct inode, and
the struct ima_iint_cache is supposed to have the same lifetime as a
struct inode (technically they die together but don't need to be created
at the same time) we don't have to worry about the ima_iint_cache
outliving or dieing before the inode.  So the refcnt isn't useful.  Just
get rid of it and free the structure when the inode is freed.

Signed-off-by: Eric Paris <eapris@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima.h      |  4 +---
 security/integrity/ima/ima_iint.c | 38 ++++++++++++++++----------------------
 security/integrity/ima/ima_main.c |  7 ++-----
 3 files changed, 19 insertions(+), 30 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 27849e1656d..ac79032bdf2 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -107,7 +107,6 @@ struct ima_iint_cache {
 	unsigned char flags;
 	u8 digest[IMA_DIGEST_SIZE];
 	struct mutex mutex;	/* protects: version, flags, digest */
-	struct kref refcount;	/* ima_iint_cache reference count */
 };
 
 /* LIM API function definitions */
@@ -125,8 +124,7 @@ void ima_template_show(struct seq_file *m, void *e,
  * integrity data associated with an inode.
  */
 struct ima_iint_cache *ima_iint_insert(struct inode *inode);
-struct ima_iint_cache *ima_iint_find_get(struct inode *inode);
-void iint_free(struct kref *kref);
+struct ima_iint_cache *ima_iint_find(struct inode *inode);
 
 /* IMA policy related functions */
 enum ima_hooks { FILE_CHECK = 1, FILE_MMAP, BPRM_CHECK };
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 0936a7197e4..969a1c1cb33 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -53,24 +53,26 @@ static struct ima_iint_cache *__ima_iint_find(struct inode *inode)
 }
 
 /*
- * ima_iint_find_get - return the iint associated with an inode
- *
- * ima_iint_find_get gets a reference to the iint. Caller must
- * remember to put the iint reference.
+ * ima_iint_find - return the iint associated with an inode
  */
-struct ima_iint_cache *ima_iint_find_get(struct inode *inode)
+struct ima_iint_cache *ima_iint_find(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
-	if (iint)
-		kref_get(&iint->refcount);
 	spin_unlock(&ima_iint_lock);
 
 	return iint;
 }
 
+static void iint_free(struct ima_iint_cache *iint)
+{
+	iint->version = 0;
+	iint->flags = 0UL;
+	kmem_cache_free(iint_cache, iint);
+}
+
 /**
  * ima_inode_alloc - allocate an iint associated with an inode
  * @inode: pointer to the inode
@@ -113,19 +115,9 @@ int ima_inode_alloc(struct inode *inode)
 	return 0;
 out_err:
 	spin_unlock(&ima_iint_lock);
-	kref_put(&new_iint->refcount, iint_free);
-	return rc;
-}
+	iint_free(new_iint);
 
-/* iint_free - called when the iint refcount goes to zero */
-void iint_free(struct kref *kref)
-{
-	struct ima_iint_cache *iint = container_of(kref, struct ima_iint_cache,
-						   refcount);
-	iint->version = 0;
-	iint->flags = 0UL;
-	kref_init(&iint->refcount);
-	kmem_cache_free(iint_cache, iint);
+	return rc;
 }
 
 /**
@@ -148,8 +140,11 @@ void ima_inode_free(struct inode *inode)
 	if (iint)
 		rb_erase(&iint->rb_node, &ima_iint_tree);
 	spin_unlock(&ima_iint_lock);
-	if (iint)
-		kref_put(&iint->refcount, iint_free);
+
+	if (!iint)
+		return;
+
+	iint_free(iint);
 }
 
 static void init_once(void *foo)
@@ -160,7 +155,6 @@ static void init_once(void *foo)
 	iint->version = 0;
 	iint->flags = 0UL;
 	mutex_init(&iint->mutex);
-	kref_init(&iint->refcount);
 }
 
 static int __init ima_iintcache_init(void)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5e3229c797f..1dccafef749 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -186,8 +186,6 @@ static void ima_file_free_iint(struct ima_iint_cache *iint, struct inode *inode,
 
 	spin_unlock(&inode->i_lock);
 	mutex_unlock(&iint->mutex);
-
-	kref_put(&iint->refcount, iint_free);
 }
 
 static void ima_file_free_noiint(struct inode *inode, struct file *file)
@@ -213,7 +211,7 @@ void ima_file_free(struct file *file)
 
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
-	iint = ima_iint_find_get(inode);
+	iint = ima_iint_find(inode);
 
 	if (iint)
 		ima_file_free_iint(iint, inode, file);
@@ -236,7 +234,7 @@ static int process_measurement(struct file *file, const unsigned char *filename,
 	if (rc != 0)
 		return rc;
 retry:
-	iint = ima_iint_find_get(inode);
+	iint = ima_iint_find(inode);
 	if (!iint) {
 		rc = ima_inode_alloc(inode);
 		if (!rc || rc == -EEXIST)
@@ -255,7 +253,6 @@ retry:
 		ima_store_measurement(iint, file, filename);
 out:
 	mutex_unlock(&iint->mutex);
-	kref_put(&iint->refcount, iint_free);
 	return rc;
 }
 
-- 
cgit v1.2.3-70-g09d2


From 196f518128d2ee6e0028b50e6fec0313640db142 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:42:19 -0400
Subject: IMA: explicit IMA i_flag to remove global lock on inode_delete

Currently for every removed inode IMA must take a global lock and search
the IMA rbtree looking for an associated integrity structure.  Instead
we explicitly mark an inode when we add an integrity structure so we
only have to take the global lock and do the removal if it exists.

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 include/linux/fs.h                |  2 ++
 security/integrity/ima/ima_iint.c | 16 +++++++++++-----
 security/integrity/ima/ima_main.c |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

(limited to 'security')

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 01e3a0047fe..bb77843de9d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -235,6 +235,7 @@ struct inodes_stat_t {
 #define S_NOCMTIME	128	/* Do not update file c/mtime */
 #define S_SWAPFILE	256	/* Do not truncate: swapon got its bmaps */
 #define S_PRIVATE	512	/* Inode is fs-internal */
+#define S_IMA		1024	/* Inode has an associated IMA struct */
 
 /*
  * Note that nosuid etc flags are inode-specific: setting some file-system
@@ -269,6 +270,7 @@ struct inodes_stat_t {
 #define IS_NOCMTIME(inode)	((inode)->i_flags & S_NOCMTIME)
 #define IS_SWAPFILE(inode)	((inode)->i_flags & S_SWAPFILE)
 #define IS_PRIVATE(inode)	((inode)->i_flags & S_PRIVATE)
+#define IS_IMA(inode)		((inode)->i_flags & S_IMA)
 
 /* the read-only stuff doesn't really belong here, but any other place is
    probably as bad and I don't want to create yet another include file. */
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 969a1c1cb33..c442e47b678 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -59,6 +59,9 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
+	if (!IS_IMA(inode))
+		return NULL;
+
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
 	spin_unlock(&ima_iint_lock);
@@ -91,6 +94,7 @@ int ima_inode_alloc(struct inode *inode)
 	new_iint->inode = inode;
 	new_node = &new_iint->rb_node;
 
+	mutex_lock(&inode->i_mutex); /* i_flags */
 	spin_lock(&ima_iint_lock);
 
 	p = &ima_iint_tree.rb_node;
@@ -107,14 +111,17 @@ int ima_inode_alloc(struct inode *inode)
 			goto out_err;
 	}
 
+	inode->i_flags |= S_IMA;
 	rb_link_node(new_node, parent, p);
 	rb_insert_color(new_node, &ima_iint_tree);
 
 	spin_unlock(&ima_iint_lock);
+	mutex_unlock(&inode->i_mutex); /* i_flags */
 
 	return 0;
 out_err:
 	spin_unlock(&ima_iint_lock);
+	mutex_unlock(&inode->i_mutex); /* i_flags */
 	iint_free(new_iint);
 
 	return rc;
@@ -135,15 +142,14 @@ void ima_inode_free(struct inode *inode)
 
 	inode->i_readcount = 0;
 
+	if (!IS_IMA(inode))
+		return;
+
 	spin_lock(&ima_iint_lock);
 	iint = __ima_iint_find(inode);
-	if (iint)
-		rb_erase(&iint->rb_node, &ima_iint_tree);
+	rb_erase(&iint->rb_node, &ima_iint_tree);
 	spin_unlock(&ima_iint_lock);
 
-	if (!iint)
-		return;
-
 	iint_free(iint);
 }
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1dccafef749..60dd61527b1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -211,6 +211,7 @@ void ima_file_free(struct file *file)
 
 	if (!iint_initialized || !S_ISREG(inode->i_mode))
 		return;
+
 	iint = ima_iint_find(inode);
 
 	if (iint)
-- 
cgit v1.2.3-70-g09d2


From bade72d607c4eb1b1d6c7852c493b75f065a56b5 Mon Sep 17 00:00:00 2001
From: Eric Paris <eparis@redhat.com>
Date: Mon, 25 Oct 2010 14:42:25 -0400
Subject: IMA: fix the ToMToU logic

Current logic looks like this:

        rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
        if (rc < 0)
                goto out;

        if (mode & FMODE_WRITE) {
                if (inode->i_readcount)
                        send_tomtou = true;
                goto out;
        }

        if (atomic_read(&inode->i_writecount) > 0)
                send_writers = true;

Lets assume we have a policy which states that all files opened for read
by root must be measured.

Lets assume the file has permissions 777.

Lets assume that root has the given file open for read.

Lets assume that a non-root process opens the file write.

The non-root process will get to ima_counts_get() and will check the
ima_must_measure().  Since it is not supposed to measure it will goto
out.

We should check the i_readcount no matter what since we might be causing
a ToMToU voilation!

This is close to correct, but still not quite perfect.  The situation
could have been that root, which was interested in the mesurement opened
and closed the file and another process which is not interested in the
measurement is the one holding the i_readcount ATM.  This is just overly
strict on ToMToU violations, which is better than not strict enough...

Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 security/integrity/ima/ima_main.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

(limited to 'security')

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 60dd61527b1..203de979d30 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -112,22 +112,23 @@ void ima_counts_get(struct file *file)
 	if (!ima_initialized)
 		goto out;
 
-	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
-	if (rc < 0)
-		goto out;
-
 	if (mode & FMODE_WRITE) {
-		if (inode->i_readcount)
+		if (inode->i_readcount && IS_IMA(inode))
 			send_tomtou = true;
 		goto out;
 	}
 
+	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
+	if (rc < 0)
+		goto out;
+
 	if (atomic_read(&inode->i_writecount) > 0)
 		send_writers = true;
 out:
 	/* remember the vfs deals with i_writecount */
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		inode->i_readcount++;
+
 	spin_unlock(&inode->i_lock);
 
 	if (send_tomtou)
-- 
cgit v1.2.3-70-g09d2