diff mbox series

[v6,02/17] ima: Define ns_status for storing namespaced iint data

Message ID 20211210194736.1538863-3-stefanb@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series ima: Namespace IMA with audit support in IMA-ns | expand

Commit Message

Stefan Berger Dec. 10, 2021, 7:47 p.m. UTC 
From: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>

This patch adds an rbtree to the IMA namespace structure that stores a
namespaced version of iint->flags in ns_status struct. Similar to the
integrity_iint_cache, both the iint ns_struct are looked up using the
inode pointer value. The lookup, allocate, and insertion code is also
similar, except ns_struct is not free'd when the inode is free'd.
Instead, the lookup verifies the i_ino and i_generation fields are also a
match.

Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Changelog:
v2:
 * fixed tree traversal in __ima_ns_status_find()
---
 include/linux/ima.h                      |   4 +-
 security/integrity/ima/Makefile          |   1 +
 security/integrity/ima/ima.h             |  24 +++++
 security/integrity/ima/ima_init_ima_ns.c |   7 ++
 security/integrity/ima/ima_ns.c          |   1 +
 security/integrity/ima/ima_ns_status.c   | 132 +++++++++++++++++++++++
 6 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100644 security/integrity/ima/ima_ns_status.c

Comments

Mimi Zohar Dec. 15, 2021, 9:12 p.m. UTC | #1 
Hi Stefan,

On Fri, 2021-12-10 at 14:47 -0500, Stefan Berger wrote:
> From: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
> 
> This patch adds an rbtree to the IMA namespace structure that stores a
> namespaced version of iint->flags in ns_status struct. Similar to the
> integrity_iint_cache, both the iint ns_struct are looked up using the
> inode pointer value. The lookup, allocate, and insertion code is also
> similar, except ns_struct is not free'd when the inode is free'd.
> Instead, the lookup verifies the i_ino and i_generation fields are also a
> match.
> 
> Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Again, hopefully it isn't premature for generic comments:

- Function/inline comments would be appreciated, especially when the
code differs from the original code.  Example below.

> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
> index f820686baf9f..08781a44f7bf 100644
> --- a/security/integrity/ima/ima_init_ima_ns.c
> +++ b/security/integrity/ima/ima_init_ima_ns.c
> @@ -14,11 +14,18 @@
>  #include <linux/user_namespace.h>
>  #include <linux/ima.h>
>  #include <linux/proc_ns.h>
> +#include <linux/slab.h>
>  
>  #include "ima.h"
>  
>  int ima_init_namespace(struct ima_namespace *ns)
>  {
> +	ns->ns_status_tree = RB_ROOT;
> +	rwlock_init(&ns->ns_status_lock);
> +	ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
> +	if (!ns->ns_status_cache)
> +		return -ENOMEM;
> +

For example, using KMEM_CACHE() is probably correct here, at least for
now, but it is different than the original code which uses
kmem_cache_alloc() with init_once().  Memory cleanup is done on free,
before it is re-used.

thanks,

Mimi

>  	return 0;
>  }
Stefan Berger Dec. 16, 2021, 2:37 a.m. UTC | #2 
On 12/15/21 16:12, Mimi Zohar wrote:
> Hi Stefan,
>
> On Fri, 2021-12-10 at 14:47 -0500, Stefan Berger wrote:
>> From: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
>>
>> This patch adds an rbtree to the IMA namespace structure that stores a
>> namespaced version of iint->flags in ns_status struct. Similar to the
>> integrity_iint_cache, both the iint ns_struct are looked up using the
>> inode pointer value. The lookup, allocate, and insertion code is also
>> similar, except ns_struct is not free'd when the inode is free'd.
>> Instead, the lookup verifies the i_ino and i_generation fields are also a
>> match.
>>
>> Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Again, hopefully it isn't premature for generic comments:
>
> - Function/inline comments would be appreciated, especially when the
> code differs from the original code.  Example below.
>
>> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
>> index f820686baf9f..08781a44f7bf 100644
>> --- a/security/integrity/ima/ima_init_ima_ns.c
>> +++ b/security/integrity/ima/ima_init_ima_ns.c
>> @@ -14,11 +14,18 @@
>>   #include <linux/user_namespace.h>
>>   #include <linux/ima.h>
>>   #include <linux/proc_ns.h>
>> +#include <linux/slab.h>
>>   
>>   #include "ima.h"
>>   
>>   int ima_init_namespace(struct ima_namespace *ns)
>>   {
>> +	ns->ns_status_tree = RB_ROOT;
>> +	rwlock_init(&ns->ns_status_lock);
>> +	ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
>> +	if (!ns->ns_status_cache)
>> +		return -ENOMEM;
>> +
> For example, using KMEM_CACHE() is probably correct here, at least for
> now, but it is different than the original code which uses
> kmem_cache_alloc() with init_once().  Memory cleanup is done on free,
> before it is re-used.

KMEM_CACHE + kmem_cache_alloc/zalloc() are pretty common. What kind of 
comment would be helpful here?

    Stefan

>
> thanks,
>
> Mimi
>
>>   	return 0;
>>   }
Mimi Zohar Dec. 16, 2021, 3:53 a.m. UTC | #3 
On Wed, 2021-12-15 at 21:37 -0500, Stefan Berger wrote:
> On 12/15/21 16:12, Mimi Zohar wrote:
> > Hi Stefan,
> >
> > On Fri, 2021-12-10 at 14:47 -0500, Stefan Berger wrote:
> >> From: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
> >>
> >> This patch adds an rbtree to the IMA namespace structure that stores a
> >> namespaced version of iint->flags in ns_status struct. Similar to the
> >> integrity_iint_cache, both the iint ns_struct are looked up using the
> >> inode pointer value. The lookup, allocate, and insertion code is also
> >> similar, except ns_struct is not free'd when the inode is free'd.
> >> Instead, the lookup verifies the i_ino and i_generation fields are also a
> >> match.
> >>
> >> Signed-off-by: Mehmet Kayaalp <mkayaalp@linux.vnet.ibm.com>
> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > Again, hopefully it isn't premature for generic comments:
> >
> > - Function/inline comments would be appreciated, especially when the
> > code differs from the original code.  Example below.
> >
> >> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
> >> index f820686baf9f..08781a44f7bf 100644
> >> --- a/security/integrity/ima/ima_init_ima_ns.c
> >> +++ b/security/integrity/ima/ima_init_ima_ns.c
> >> @@ -14,11 +14,18 @@
> >>   #include <linux/user_namespace.h>
> >>   #include <linux/ima.h>
> >>   #include <linux/proc_ns.h>
> >> +#include <linux/slab.h>
> >>   
> >>   #include "ima.h"
> >>   
> >>   int ima_init_namespace(struct ima_namespace *ns)
> >>   {
> >> +	ns->ns_status_tree = RB_ROOT;
> >> +	rwlock_init(&ns->ns_status_lock);
> >> +	ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
> >> +	if (!ns->ns_status_cache)
> >> +		return -ENOMEM;
> >> +
> > For example, using KMEM_CACHE() is probably correct here, at least for
> > now, but it is different than the original code which uses
> > kmem_cache_alloc() with init_once().  Memory cleanup is done on free,

^  kmem_cache_create() with init_once.

> > before it is re-used.
> 
> KMEM_CACHE + kmem_cache_alloc/zalloc() are pretty common. What kind of 
> comment would be helpful here?

The original reason for using kmem_cache_create() with init_once and
deferring memory cleanup to free was for performance.  Using
KMEM_CACHE() and kmem_cache_zalloc() instead could be for simplicity. 
The comment should note the change.

Mimi
Stefan Berger Dec. 16, 2021, 4 a.m. UTC | #4 
On 12/15/21 22:53, Mimi Zohar wrote:
> On Wed, 2021-12-15 at 21:37 -0500, Stefan Berger wrote:
>
>>> before it is re-used.
>> KMEM_CACHE + kmem_cache_alloc/zalloc() are pretty common. What kind of
>> comment would be helpful here?
> The original reason for using kmem_cache_create() with init_once and
> deferring memory cleanup to free was for performance.  Using
> KMEM_CACHE() and kmem_cache_zalloc() instead could be for simplicity.
> The comment should note the change.

/* use KMEM_CACHE for simplicity */ ?


>
> Mimi
>
diff mbox series

Patch

diff --git a/include/linux/ima.h b/include/linux/ima.h
index f282e40c316c..19e6936250ea 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -212,7 +212,9 @@  static inline int ima_inode_removexattr(struct dentry *dentry,
 #endif /* CONFIG_IMA_APPRAISE */
 
 struct ima_namespace {
-	int avoid_zero_size;
+	struct rb_root ns_status_tree;
+	rwlock_t ns_status_lock;
+	struct kmem_cache *ns_status_cache;
 };
 
 extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index b86a35fbed60..78c84214e109 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -10,6 +10,7 @@  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 	 ima_policy.o ima_template.o ima_template_lib.o ima_init_ima_ns.o
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
 ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
+ima-$(CONFIG_IMA_NS) += ima_ns.o ima_ns_status.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
 ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
 ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2f8adf383054..28896d256e36 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -133,6 +133,14 @@  static inline void ima_load_kexec_buffer(void) {}
  */
 extern bool ima_canonical_fmt;
 
+struct ns_status {
+	struct rb_node rb_node;
+	struct inode *inode;
+	ino_t i_ino;
+	u32 i_generation;
+	unsigned long flags;
+};
+
 /* Internal IMA function definitions */
 int ima_init(void);
 int ima_fs_init(void);
@@ -422,6 +430,22 @@  int ima_ns_init(void);
 struct ima_namespace;
 int ima_init_namespace(struct ima_namespace *ns);
 
+#ifdef CONFIG_IMA_NS
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+				    struct inode *inode);
+
+void free_ns_status_cache(struct ima_namespace *ns);
+
+#else
+
+static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+						  struct inode *inode)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_IMA_NS */
+
 /* LSM based policy rules require audit */
 #ifdef CONFIG_IMA_LSM_RULES
 
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index f820686baf9f..08781a44f7bf 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -14,11 +14,18 @@ 
 #include <linux/user_namespace.h>
 #include <linux/ima.h>
 #include <linux/proc_ns.h>
+#include <linux/slab.h>
 
 #include "ima.h"
 
 int ima_init_namespace(struct ima_namespace *ns)
 {
+	ns->ns_status_tree = RB_ROOT;
+	rwlock_init(&ns->ns_status_lock);
+	ns->ns_status_cache = KMEM_CACHE(ns_status, SLAB_PANIC);
+	if (!ns->ns_status_cache)
+		return -ENOMEM;
+
 	return 0;
 }
 
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index c575ddf0c3f3..20472959ec26 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -48,6 +48,7 @@  int create_ima_ns(struct user_namespace *user_ns)
 static void destroy_ima_ns(struct ima_namespace *ns)
 {
 	pr_debug("DESTROY ima_ns: 0x%p\n", ns);
+	free_ns_status_cache(ns);
 	kmem_cache_free(imans_cachep, ns);
 }
 
diff --git a/security/integrity/ima/ima_ns_status.c b/security/integrity/ima/ima_ns_status.c
new file mode 100644
index 000000000000..807dfaecdb5e
--- /dev/null
+++ b/security/integrity/ima/ima_ns_status.c
@@ -0,0 +1,132 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2016-2021 IBM Corporation
+ * Author:
+ *  Yuqiong Sun <suny@us.ibm.com>
+ *  Stefan Berger <stefanb@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#include <linux/user_namespace.h>
+#include <linux/proc_ns.h>
+#include <linux/ima.h>
+
+#include "ima.h"
+
+void free_ns_status_cache(struct ima_namespace *ns)
+{
+	struct ns_status *status, *next;
+
+	write_lock(&ns->ns_status_lock);
+	rbtree_postorder_for_each_entry_safe(status, next,
+					     &ns->ns_status_tree, rb_node)
+		kmem_cache_free(ns->ns_status_cache, status);
+	ns->ns_status_tree = RB_ROOT;
+	write_unlock(&ns->ns_status_lock);
+	kmem_cache_destroy(ns->ns_status_cache);
+}
+
+/*
+ * __ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *__ima_ns_status_find(struct ima_namespace *ns,
+					      struct inode *inode)
+{
+	struct ns_status *status;
+	struct rb_node *n = ns->ns_status_tree.rb_node;
+
+	while (n) {
+		status = rb_entry(n, struct ns_status, rb_node);
+
+		if (inode < status->inode)
+			n = n->rb_left;
+		else if (inode > status->inode)
+			n = n->rb_right;
+		else
+			break;
+	}
+	if (!n)
+		return NULL;
+
+	return status;
+}
+
+/*
+ * ima_ns_status_find - return the ns_status associated with an inode
+ */
+static struct ns_status *ima_ns_status_find(struct ima_namespace *ns,
+					    struct inode *inode)
+{
+	struct ns_status *status;
+
+	read_lock(&ns->ns_status_lock);
+	status = __ima_ns_status_find(ns, inode);
+	read_unlock(&ns->ns_status_lock);
+
+	return status;
+}
+
+static void insert_ns_status(struct ima_namespace *ns, struct inode *inode,
+			     struct ns_status *status)
+{
+	struct rb_node **p;
+	struct rb_node *node, *parent = NULL;
+	struct ns_status *test_status;
+
+	p = &ns->ns_status_tree.rb_node;
+	while (*p) {
+		parent = *p;
+		test_status = rb_entry(parent, struct ns_status, rb_node);
+		if (inode < test_status->inode)
+			p = &(*p)->rb_left;
+		else
+			p = &(*p)->rb_right;
+	}
+	node = &status->rb_node;
+	rb_link_node(node, parent, p);
+	rb_insert_color(node, &ns->ns_status_tree);
+}
+
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+				    struct inode *inode)
+{
+	struct ns_status *status;
+	int skip_insert = 0;
+
+	status = ima_ns_status_find(ns, inode);
+	if (status) {
+		/*
+		 * Unlike integrity_iint_cache we are not free'ing the
+		 * ns_status data when the inode is free'd. So, in addition to
+		 * checking the inode pointer, we need to make sure the
+		 * (i_generation, i_ino) pair matches as well.
+		 */
+		if (inode->i_ino == status->i_ino &&
+		    inode->i_generation == status->i_generation)
+			return status;
+
+		/* Same inode number is reused, overwrite the ns_status */
+		skip_insert = 1;
+	} else {
+		status = kmem_cache_alloc(ns->ns_status_cache, GFP_NOFS);
+		if (!status)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	write_lock(&ns->ns_status_lock);
+
+	if (!skip_insert)
+		insert_ns_status(ns, inode, status);
+
+	status->inode = inode;
+	status->i_ino = inode->i_ino;
+	status->i_generation = inode->i_generation;
+	status->flags = 0UL;
+
+	write_unlock(&ns->ns_status_lock);
+
+	return status;
+}