[PATCH 0/4] IMA: making i_readcount a first class inode citizen

[PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

This is a repost of patches 1 - 4 of the ima-i_readcount patch set rebased
against security-testing/#next, as requested a while ago.  The patches are
also available from:
git://git.kernel.org/pub/scm/linux/kernel/git/zohar/ima-2.6.git/#next

This patchset separates the incrementing/decrementing of the i_readcount,
in the VFS layer, from other IMA functionality, by replacing the current
ima_counts_get() call with i_readcount_inc(). Its unclear whether this
call to increment i_readcount should be made earlier, like i_writecount.
Currently the call is situated immediately after the switch from put_filp()
to fput() for cleanup.

Mimi

Mimi Zohar (4):
  IMA: convert i_readcount to atomic
  IMA: define readcount functions
  IMA: maintain i_readcount in the VFS layer
  IMA: remove IMA imbalance checking

 fs/file_table.c                   |    5 +-
 fs/open.c                         |    3 +-
 include/linux/fs.h                |   23 ++++++-
 include/linux/ima.h               |    6 --
 security/integrity/ima/ima_iint.c |    5 --
 security/integrity/ima/ima_main.c |  130 ++++---------------------------------
 6 files changed, 40 insertions(+), 132 deletions(-)

-- 
1.7.3.4

[PATCH 1/4] IMA: convert i_readcount to atomic

[Mimi Zohar]

Convert the inode's i_readcount from an unsigned int to atomic.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 include/linux/fs.h                |    3 +--
 security/integrity/ima/ima_iint.c |    7 ++++---
 security/integrity/ima/ima_main.c |   11 ++++++-----
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index baf3e55..ef85322 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -794,8 +794,7 @@ struct inode {
 #endif
 
 #ifdef CONFIG_IMA
-	/* protected by i_lock */
-	unsigned int		i_readcount; /* struct files open RO */
+	atomic_t		i_readcount; /* struct files open RO */
 #endif
 	atomic_t		i_writecount;
 #ifdef CONFIG_SECURITY
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index c442e47..f005355 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,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);
+	if (atomic_read(&inode->i_readcount))
+		printk(KERN_INFO "%s: readcount: %u\n", __func__,
+		       atomic_read(&inode->i_readcount));
 
-	inode->i_readcount = 0;
+	atomic_set(&inode->i_readcount, 0);
 
 	if (!IS_IMA(inode))
 		return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 203de97..6e8cb93 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -113,7 +113,7 @@ void ima_counts_get(struct file *file)
 		goto out;
 
 	if (mode & FMODE_WRITE) {
-		if (inode->i_readcount && IS_IMA(inode))
+		if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
 			send_tomtou = true;
 		goto out;
 	}
@@ -127,7 +127,7 @@ void ima_counts_get(struct file *file)
 out:
 	/* remember the vfs deals with i_writecount */
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
-		inode->i_readcount++;
+		atomic_inc(&inode->i_readcount);
 
 	spin_unlock(&inode->i_lock);
 
@@ -149,15 +149,16 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
 	assert_spin_locked(&inode->i_lock);
 
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(inode->i_readcount == 0)) {
+		if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
 			if (!ima_limit_imbalance(file)) {
 				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-				       __func__, inode->i_readcount);
+				       __func__,
+				       atomic_read(&inode->i_readcount));
 				dump_stack();
 			}
 			return;
 		}
-		inode->i_readcount--;
+		atomic_dec(&inode->i_readcount);
 	}
 }
 
-- 
1.7.3.4

[PATCH 2/4] IMA: define readcount functions

[Mimi Zohar]

Define i_readcount_inc/dec() functions to be called from the VFS layer.

Changelog:
- renamed iget/iput_readcount to i_readcount_inc/dec (Dave Chinner's suggestion)
- removed i_lock in iput_readcount() (based on comments:Dave Chinner,Eric Paris)

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 include/linux/fs.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index ef85322..a3e8f02 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2186,6 +2186,26 @@ static inline void allow_write_access(struct file *file)
 	if (file)
 		atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
 }
+#ifdef CONFIG_IMA
+static inline void i_readcount_dec(struct inode *inode)
+{
+	BUG_ON(!atomic_read(&inode->i_readcount));
+	atomic_dec(&inode->i_readcount);
+}
+static inline void i_readcount_inc(struct inode *inode)
+{
+	atomic_inc(&inode->i_readcount);
+}
+#else
+static inline void i_readcount_dec(struct inode *inode)
+{
+	return;
+}
+static inline void i_readcount_inc(struct inode *inode)
+{
+	return;
+}
+#endif
 extern int do_pipe_flags(int *, int);
 extern struct file *create_read_pipe(struct file *f, int flags);
 extern struct file *create_write_pipe(int flags);
-- 
1.7.3.4

[PATCH 3/4] IMA: maintain i_readcount in the VFS layer

[Mimi Zohar]

ima_counts_get() updated the readcount and invalidated the PCR,
as necessary. Only update the i_readcount in the VFS layer.
Move the PCR invalidation checks to ima_file_check(), where it
belongs.

Maintaining the i_readcount in the VFS layer, will allow other
subsystems to use i_readcount.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 fs/file_table.c                   |    5 ++++-
 fs/open.c                         |    3 ++-
 include/linux/ima.h               |    6 ------
 security/integrity/ima/ima_iint.c |    2 --
 security/integrity/ima/ima_main.c |   25 ++++++++-----------------
 5 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index c3dee38..0c724de 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -190,7 +190,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
 		file_take_write(file);
 		WARN_ON(mnt_clone_write(path->mnt));
 	}
-	ima_counts_get(file);
+	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		i_readcount_inc(path->dentry->d_inode);
 	return file;
 }
 EXPORT_SYMBOL(alloc_file);
@@ -251,6 +252,8 @@ static void __fput(struct file *file)
 	fops_put(file->f_op);
 	put_pid(file->f_owner.pid);
 	file_sb_list_del(file);
+	if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		i_readcount_dec(inode);
 	if (file->f_mode & FMODE_WRITE)
 		drop_file_write_access(file);
 	file->f_path.dentry = NULL;
diff --git a/fs/open.c b/fs/open.c
index 4197b9e..0d485c5 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -688,7 +688,8 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 		if (error)
 			goto cleanup_all;
 	}
-	ima_counts_get(f);
+	if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+		i_readcount_inc(inode);
 
 	f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
 
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 975837e..09e6e62 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,6 @@ extern void ima_inode_free(struct inode *inode);
 extern int ima_file_check(struct file *file, int mask);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
-extern void ima_counts_get(struct file *file);
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -53,10 +52,5 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
 	return 0;
 }
 
-static inline void ima_counts_get(struct file *file)
-{
-	return;
-}
-
 #endif /* CONFIG_IMA_H */
 #endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index f005355..68efe3b 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -141,8 +141,6 @@ void ima_inode_free(struct inode *inode)
 		printk(KERN_INFO "%s: readcount: %u\n", __func__,
 		       atomic_read(&inode->i_readcount));
 
-	atomic_set(&inode->i_readcount, 0);
-
 	if (!IS_IMA(inode))
 		return;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6e8cb93..69b4856 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -86,17 +86,16 @@ out:
 }
 
 /*
- * ima_counts_get - increment file counts
+ * ima_rdwr_violation_check
  *
- * Maintain read/write counters for all files, but only
- * invalidate the PCR for measured files:
+ * Only invalidate the PCR for measured files:
  * 	- Opening a file for write when already open for read,
  *	  results in a time of measure, time of use (ToMToU) error.
  *	- Opening a file for read when already open for write,
  * 	  could result in a file measurement error.
  *
  */
-void ima_counts_get(struct file *file)
+static void ima_rdwr_violation_check(struct file *file)
 {
 	struct dentry *dentry = file->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
@@ -104,13 +103,10 @@ void ima_counts_get(struct file *file)
 	int rc;
 	bool send_tomtou = false, send_writers = false;
 
-	if (!S_ISREG(inode->i_mode))
+	if (!S_ISREG(inode->i_mode) || !ima_initialized)
 		return;
 
-	spin_lock(&inode->i_lock);
-
-	if (!ima_initialized)
-		goto out;
+	mutex_lock(&inode->i_mutex);	/* file metadata: permissions, xattr */
 
 	if (mode & FMODE_WRITE) {
 		if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
@@ -125,11 +121,7 @@ void ima_counts_get(struct file *file)
 	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)
-		atomic_inc(&inode->i_readcount);
-
-	spin_unlock(&inode->i_lock);
+	mutex_unlock(&inode->i_mutex);
 
 	if (send_tomtou)
 		ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
@@ -158,7 +150,6 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
 			}
 			return;
 		}
-		atomic_dec(&inode->i_readcount);
 	}
 }
 
@@ -203,8 +194,7 @@ static void ima_file_free_noiint(struct inode *inode, struct file *file)
  * ima_file_free - called on __fput()
  * @file: pointer to file structure being freed
  *
- * Flag files that changed, based on i_version;
- * and decrement the i_readcount.
+ * Flag files that changed, based on i_version
  */
 void ima_file_free(struct file *file)
 {
@@ -318,6 +308,7 @@ int ima_file_check(struct file *file, int mask)
 {
 	int rc;
 
+	ima_rdwr_violation_check(file);
 	rc = process_measurement(file, file->f_dentry->d_name.name,
 				 mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
 				 FILE_CHECK);
-- 
1.7.3.4

[PATCH 4/4] IMA: remove IMA imbalance checking

[Mimi Zohar]

Now that i_readcount is maintained by the VFS layer, remove the
imbalance checking in IMA. Cleans up the IMA code nicely.

Signed-off-by: Mimi Zohar <zohar@us.ibm.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 security/integrity/ima/ima_iint.c |    4 --
 security/integrity/ima/ima_main.c |  104 ++-----------------------------------
 2 files changed, 4 insertions(+), 104 deletions(-)

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 68efe3b..4ae7304 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,6 @@ void ima_inode_free(struct inode *inode)
 {
 	struct ima_iint_cache *iint;
 
-	if (atomic_read(&inode->i_readcount))
-		printk(KERN_INFO "%s: readcount: %u\n", __func__,
-		       atomic_read(&inode->i_readcount));
-
 	if (!IS_IMA(inode))
 		return;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 69b4856..2df9021 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -36,55 +36,6 @@ static int __init hash_setup(char *str)
 }
 __setup("ima_hash=", hash_setup);
 
-struct ima_imbalance {
-	struct hlist_node node;
-	unsigned long fsmagic;
-};
-
-/*
- * ima_limit_imbalance - emit one imbalance message per filesystem type
- *
- * Maintain list of filesystem types that do not measure files properly.
- * Return false if unknown, true if known.
- */
-static bool ima_limit_imbalance(struct file *file)
-{
-	static DEFINE_SPINLOCK(ima_imbalance_lock);
-	static HLIST_HEAD(ima_imbalance_list);
-
-	struct super_block *sb = file->f_dentry->d_sb;
-	struct ima_imbalance *entry;
-	struct hlist_node *node;
-	bool found = false;
-
-	rcu_read_lock();
-	hlist_for_each_entry_rcu(entry, node, &ima_imbalance_list, node) {
-		if (entry->fsmagic == sb->s_magic) {
-			found = true;
-			break;
-		}
-	}
-	rcu_read_unlock();
-	if (found)
-		goto out;
-
-	entry = kmalloc(sizeof(*entry), GFP_NOFS);
-	if (!entry)
-		goto out;
-	entry->fsmagic = sb->s_magic;
-	spin_lock(&ima_imbalance_lock);
-	/*
-	 * we could have raced and something else might have added this fs
-	 * to the list, but we don't really care
-	 */
-	hlist_add_head_rcu(&entry->node, &ima_imbalance_list);
-	spin_unlock(&ima_imbalance_lock);
-	printk(KERN_INFO "IMA: unmeasured files on fsmagic: %lX\n",
-	       entry->fsmagic);
-out:
-	return found;
-}
-
 /*
  * ima_rdwr_violation_check
  *
@@ -131,65 +82,20 @@ out:
 				  "open_writers");
 }
 
-/*
- * Decrement ima counts
- */
-static void ima_dec_counts(struct inode *inode, struct file *file)
-{
-	mode_t mode = file->f_mode;
-
-	assert_spin_locked(&inode->i_lock);
-
-	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-		if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
-			if (!ima_limit_imbalance(file)) {
-				printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
-				       __func__,
-				       atomic_read(&inode->i_readcount));
-				dump_stack();
-			}
-			return;
-		}
-	}
-}
-
 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);
-
+	mutex_lock(&iint->mutex);
 	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);
 }
 
-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);
-}
-
 /**
  * ima_file_free - called on __fput()
  * @file: pointer to file structure being freed
@@ -205,12 +111,10 @@ void ima_file_free(struct file *file)
 		return;
 
 	iint = ima_iint_find(inode);
+	if (!iint)
+		return;
 
-	if (iint)
-		ima_file_free_iint(iint, inode, file);
-	else
-		ima_file_free_noiint(inode, file);
-
+	ima_check_last_writer(iint, inode, file);
 }
 
 static int process_measurement(struct file *file, const unsigned char *filename,
-- 
1.7.3.4

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[Mimi Zohar]

Hi, 

As Linus has already agreed that the i_readcount patches (1 - 4) can be
upstreamed (https://lkml.org/lkml/2010/11/18/670), do I need to do
anything else (eg. send James a request to pull)?

thanks,

Mimi

On Mon, 2011-02-14 at 15:29 -0500, Mimi Zohar wrote:
> This is a repost of patches 1 - 4 of the ima-i_readcount patch set rebased
> against security-testing/#next, as requested a while ago.  The patches are
> also available from:
> git://git.kernel.org/pub/scm/linux/kernel/git/zohar/ima-2.6.git/#next
> 
> This patchset separates the incrementing/decrementing of the i_readcount,
> in the VFS layer, from other IMA functionality, by replacing the current
> ima_counts_get() call with i_readcount_inc(). Its unclear whether this
> call to increment i_readcount should be made earlier, like i_writecount.
> Currently the call is situated immediately after the switch from put_filp()
> to fput() for cleanup.
> 
> Mimi
> 
> Mimi Zohar (4):
>   IMA: convert i_readcount to atomic
>   IMA: define readcount functions
>   IMA: maintain i_readcount in the VFS layer
>   IMA: remove IMA imbalance checking
> 
>  fs/file_table.c                   |    5 +-
>  fs/open.c                         |    3 +-
>  include/linux/fs.h                |   23 ++++++-
>  include/linux/ima.h               |    6 --
>  security/integrity/ima/ima_iint.c |    5 --
>  security/integrity/ima/ima_main.c |  130 ++++---------------------------------
>  6 files changed, 40 insertions(+), 132 deletions(-)
> 

Re: [PATCH 0/4] IMA: making i_readcount a first class inode citizen

[James Morris]

On Wed, 16 Feb 2011, Mimi Zohar wrote:

> Hi, 
> 
> As Linus has already agreed that the i_readcount patches (1 - 4) can be
> upstreamed (https://lkml.org/lkml/2010/11/18/670), do I need to do
> anything else (eg. send James a request to pull)?


I've pulled these changes into
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

-- 
James Morris
<jmorris@namei.org>