From: Mehmet Kayaalp <[email protected]>
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 <[email protected]>
Signed-off-by: Stefan Berger <[email protected]>
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
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 <[email protected]>
+ * Stefan Berger <[email protected]>
+ *
+ * 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;
+}
--
2.31.1
Hi Stefan,
On Fri, 2021-12-10 at 14:47 -0500, Stefan Berger wrote:
> From: Mehmet Kayaalp <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Stefan Berger <[email protected]>
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;
> }
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Stefan Berger <[email protected]>
> 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;
>> }
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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: Stefan Berger <[email protected]>
> > 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
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
>