Menu
▾
▴
linux-ima-devel
|
From: Mehmet K. <mka...@li...> - 2017-07-20 22:53:08
|
This patch set implements an IMA namespace data structure that gets created alongside a mount namespace with CLONE_NEWNS, and lays down the foundation for namespacing the different aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal). The original PoC patches [1], created a new CLONE_NEWIMA flag to explicitly control when a new IMA namespace should be created. Based on comments, we elected to hang the IMA namepace off of existing namespaces, and the mount namespace made the most sense. However, we actually allocate a new namespace struct in nsproxy, allocate a new inum, and have an ima symlink in /proc/*/ns/, instead of adding a pointer from the mnt_namespace. As a result, clone() and unshare() with CLONE_NEWNS results in a new mount and a new IMA namespace, while setns() called with the fd of /proc/*/ns/mnt would NOT have the same result. A second setns() call with the fd /proc/*/ns/ima would be required. The first patch creates the ima_namespace data, while the second patch puts the iint->flags in the namespace. The third patch uses these flags for namespacing the IMA-audit messages, enabling the same file to be audited each time it is accessed in a new namespace. Rest of the patches are small fixes and improvements to the audit messages generated by IMA. Subsequent patch sets will namespace IMA-measurement and IMA-appraisal. [1] https://sourceforge.net/p/linux-ima/mailman/message/35939754/ Guilherme Magalhaes (1): ima: Add ns_mnt, dev, ino fields to IMA audit measurement msgs Mehmet Kayaalp (2): ima: Add ns_status for storing namespaced iint data ima: mamespace audit status flags Mimi Zohar (1): ima: differentiate auditing policy rules from "audit" actions Yuqiong Sun (1): ima: extend clone() with IMA namespace support fs/proc/namespaces.c | 3 + include/linux/ima.h | 40 +++++ include/linux/nsproxy.h | 1 + include/linux/proc_ns.h | 2 + include/uapi/linux/audit.h | 3 +- init/Kconfig | 10 ++ kernel/nsproxy.c | 15 ++ security/integrity/ima/Makefile | 1 + security/integrity/ima/ima.h | 49 +++++- security/integrity/ima/ima_api.c | 18 +- security/integrity/ima/ima_init.c | 4 + security/integrity/ima/ima_main.c | 15 +- security/integrity/ima/ima_ns.c | 324 ++++++++++++++++++++++++++++++++++++ security/integrity/ima/ima_policy.c | 2 +- 14 files changed, 478 insertions(+), 9 deletions(-) create mode 100644 security/integrity/ima/ima_ns.c -- 2.9.4 |
|
From: Mehmet K. <mka...@li...> - 2017-07-20 22:54:04
|
From: Yuqiong Sun <su...@us...>
Add new CONFIG_IMA_NS config option. Let clone() create a new IMA
namespace upon CLONE_NEWNS flag. Add ima_ns data structure in nsproxy.
ima_ns is allocated and freed upon IMA namespace creation and exit.
Currently, the ima_ns contains no useful IMA data but only a dummy
interface. This patch creates the framework for namespacing the different
aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal).
Signed-off-by: Yuqiong Sun <su...@us...>
Changelog:
* Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
* Use existing ima.h headers
* Move the ima_namespace.c to security/integrity/ima/ima_ns.c
* Fix typo INFO->INO
* Each namespace free's itself, removed recursively free'ing
until init_ima_ns from free_ima_ns()
Signed-off-by: Mehmet Kayaalp <mka...@li...>
---
fs/proc/namespaces.c | 3 +
include/linux/ima.h | 37 ++++++++
include/linux/nsproxy.h | 1 +
include/linux/proc_ns.h | 2 +
init/Kconfig | 8 ++
kernel/nsproxy.c | 15 ++++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 9 ++
security/integrity/ima/ima_init.c | 4 +
security/integrity/ima/ima_ns.c | 183 ++++++++++++++++++++++++++++++++++++++
10 files changed, 263 insertions(+)
create mode 100644 security/integrity/ima/ima_ns.c
diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
index 3803b24..222a64e 100644
--- a/fs/proc/namespaces.c
+++ b/fs/proc/namespaces.c
@@ -32,6 +32,9 @@ static const struct proc_ns_operations *ns_entries[] = {
#ifdef CONFIG_CGROUPS
&cgroupns_operations,
#endif
+#ifdef CONFIG_IMA_NS
+ &imans_operations,
+#endif
};
static const char *proc_ns_get_link(struct dentry *dentry,
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e..11e4841 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -105,4 +105,41 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
return 0;
}
#endif /* CONFIG_IMA_APPRAISE */
+
+struct ima_namespace {
+ struct kref kref;
+ struct user_namespace *user_ns;
+ struct ns_common ns;
+ struct ima_namespace *parent;
+};
+
+extern struct ima_namespace init_ima_ns;
+
+#ifdef CONFIG_IMA_NS
+void put_ima_ns(struct ima_namespace *ns);
+struct ima_namespace *copy_ima(unsigned long flags,
+ struct user_namespace *user_ns,
+ struct ima_namespace *old_ns);
+static inline struct ima_namespace *get_current_ns(void)
+{
+ return current->nsproxy->ima_ns;
+}
+#else
+static inline void put_ima_ns(struct ima_namespace *ns)
+{
+ return;
+}
+
+static inline struct ima_namespace *copy_ima(unsigned long flags,
+ struct user_namespace *user_ns,
+ struct ima_namespace *old_ns)
+{
+ return old_ns;
+}
+
+static inline struct ima_namespace *get_current_ns(void)
+{
+ return NULL;
+}
+#endif /* CONFIG_IMA_NS */
#endif /* _LINUX_IMA_H */
diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
index ac0d65b..a97031d 100644
--- a/include/linux/nsproxy.h
+++ b/include/linux/nsproxy.h
@@ -35,6 +35,7 @@ struct nsproxy {
struct pid_namespace *pid_ns_for_children;
struct net *net_ns;
struct cgroup_namespace *cgroup_ns;
+ struct ima_namespace *ima_ns;
};
extern struct nsproxy init_nsproxy;
diff --git a/include/linux/proc_ns.h b/include/linux/proc_ns.h
index 58ab28d..c7c1239 100644
--- a/include/linux/proc_ns.h
+++ b/include/linux/proc_ns.h
@@ -31,6 +31,7 @@ extern const struct proc_ns_operations pidns_for_children_operations;
extern const struct proc_ns_operations userns_operations;
extern const struct proc_ns_operations mntns_operations;
extern const struct proc_ns_operations cgroupns_operations;
+extern const struct proc_ns_operations imans_operations;
/*
* We always define these enumerators
@@ -42,6 +43,7 @@ enum {
PROC_USER_INIT_INO = 0xEFFFFFFDU,
PROC_PID_INIT_INO = 0xEFFFFFFCU,
PROC_CGROUP_INIT_INO = 0xEFFFFFFBU,
+ PROC_IMA_INIT_INO = 0xEFFFFFFAU,
};
#ifdef CONFIG_PROC_FS
diff --git a/init/Kconfig b/init/Kconfig
index 1d3475f..339f84d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1287,6 +1287,14 @@ config NET_NS
help
Allow user space to create what appear to be multiple instances
of the network stack.
+config IMA_NS
+ bool "IMA namespace"
+ depends on IMA
+ default y
+ help
+ Allow the creation of IMA namespaces for each mount namespace.
+ Namespaced IMA data enables having IMA features work separately
+ for each mount namespace.
endif # NAMESPACES
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index f6c5d33..85be341 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -27,6 +27,7 @@
#include <linux/syscalls.h>
#include <linux/cgroup.h>
#include <linux/perf_event.h>
+#include <linux/ima.h>
static struct kmem_cache *nsproxy_cachep;
@@ -44,6 +45,9 @@ struct nsproxy init_nsproxy = {
#ifdef CONFIG_CGROUPS
.cgroup_ns = &init_cgroup_ns,
#endif
+#ifdef CONFIG_IMA_NS
+ .ima_ns = &init_ima_ns,
+#endif
};
static inline struct nsproxy *create_nsproxy(void)
@@ -110,8 +114,17 @@ static struct nsproxy *create_new_namespaces(unsigned long flags,
goto out_net;
}
+ new_nsp->ima_ns = copy_ima(flags, user_ns, tsk->nsproxy->ima_ns);
+ if (IS_ERR(new_nsp->ima_ns)) {
+ err = PTR_ERR(new_nsp->ima_ns);
+ goto out_ima;
+ }
+
return new_nsp;
+out_ima:
+ if (new_nsp->ima_ns)
+ put_ima_ns(new_nsp->ima_ns);
out_net:
put_cgroup_ns(new_nsp->cgroup_ns);
out_cgroup:
@@ -181,6 +194,8 @@ void free_nsproxy(struct nsproxy *ns)
if (ns->pid_ns_for_children)
put_pid_ns(ns->pid_ns_for_children);
put_cgroup_ns(ns->cgroup_ns);
+ if (ns->ima_ns)
+ put_ima_ns(ns->ima_ns);
put_net(ns->net_ns);
kmem_cache_free(nsproxy_cachep, ns);
}
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 29f198b..20493f0 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -8,5 +8,6 @@ obj-$(CONFIG_IMA) += ima.o
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-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_NS) += ima_ns.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d52b487..8a8234a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -291,6 +291,15 @@ static inline int ima_read_xattr(struct dentry *dentry,
#endif /* CONFIG_IMA_APPRAISE */
+#ifdef CONFIG_IMA_NS
+int ima_ns_init(void);
+#else
+static inline int ima_ns_init(void)
+{
+ return 0;
+}
+#endif /* CONFIG_IMA_NS */
+
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 2967d49..7f884a7 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -137,5 +137,9 @@ int __init ima_init(void)
ima_init_policy();
+ rc = ima_ns_init();
+ if (rc != 0)
+ return rc;
+
return ima_fs_init();
}
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
new file mode 100644
index 0000000..383217b
--- /dev/null
+++ b/security/integrity/ima/ima_ns.c
@@ -0,0 +1,183 @@
+/*
+ * Copyright (C) 2008 IBM Corporation
+ * Author: Yuqiong Sun <su...@us...>
+ *
+ * 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/export.h>
+#include <linux/user_namespace.h>
+#include <linux/proc_ns.h>
+#include <linux/kref.h>
+#include <linux/slab.h>
+#include <linux/ima.h>
+
+#include "ima.h"
+
+static void get_ima_ns(struct ima_namespace *ns);
+
+int ima_init_namespace(struct ima_namespace *ns)
+{
+ return 0;
+}
+
+int ima_ns_init(void)
+{
+ return ima_init_namespace(&init_ima_ns);
+}
+
+static struct ima_namespace *create_ima_ns(void)
+{
+ struct ima_namespace *ima_ns;
+
+ ima_ns = kmalloc(sizeof(*ima_ns), GFP_KERNEL);
+ if (ima_ns)
+ kref_init(&ima_ns->kref);
+
+ return ima_ns;
+}
+
+/**
+ * Clone a new ns copying an original ima namespace, setting refcount to 1
+ *
+ * @old_ns: old ima namespace to clone
+ * @user_ns: user namespace that current task runs in
+ * Return ERR_PTR(-ENOMEM) on error (failure to kmalloc), new ns otherwise
+ */
+static struct ima_namespace *clone_ima_ns(struct user_namespace *user_ns,
+ struct ima_namespace *old_ns)
+{
+ struct ima_namespace *ns;
+ int err;
+
+ ns = create_ima_ns();
+ if (!ns)
+ return ERR_PTR(-ENOMEM);
+
+ err = ns_alloc_inum(&ns->ns);
+ if (err) {
+ kfree(ns);
+ return ERR_PTR(err);
+ }
+
+ ns->ns.ops = &imans_operations;
+ get_ima_ns(old_ns);
+ ns->parent = old_ns;
+ ns->user_ns = get_user_ns(user_ns);
+
+ ima_init_namespace(ns);
+
+ return ns;
+}
+
+/**
+ * Copy task's ima namespace, or clone it if flags specifies CLONE_NEWNS.
+ *
+ * @flags: flags used in the clone syscall
+ * @user_ns: user namespace that current task runs in
+ * @old_ns: old ima namespace to clone
+ */
+
+struct ima_namespace *copy_ima(unsigned long flags,
+ struct user_namespace *user_ns,
+ struct ima_namespace *old_ns)
+{
+ struct ima_namespace *new_ns;
+
+ BUG_ON(!old_ns);
+ get_ima_ns(old_ns);
+
+ if (!(flags & CLONE_NEWNS))
+ return old_ns;
+
+ new_ns = clone_ima_ns(user_ns, old_ns);
+ put_ima_ns(old_ns);
+
+ return new_ns;
+}
+
+static void destroy_ima_ns(struct ima_namespace *ns)
+{
+ put_user_ns(ns->user_ns);
+ ns_free_inum(&ns->ns);
+ kfree(ns);
+}
+
+static void free_ima_ns(struct kref *kref)
+{
+ struct ima_namespace *ns;
+
+ ns = container_of(kref, struct ima_namespace, kref);
+ destroy_ima_ns(ns);
+}
+
+static void get_ima_ns(struct ima_namespace *ns)
+{
+ kref_get(&ns->kref);
+}
+
+void put_ima_ns(struct ima_namespace *ns)
+{
+ kref_put(&ns->kref, free_ima_ns);
+}
+
+static inline struct ima_namespace *to_ima_ns(struct ns_common *ns)
+{
+ return container_of(ns, struct ima_namespace, ns);
+}
+
+static struct ns_common *imans_get(struct task_struct *task)
+{
+ struct ima_namespace *ns = NULL;
+ struct nsproxy *nsproxy;
+
+ task_lock(task);
+ nsproxy = task->nsproxy;
+ if (nsproxy) {
+ ns = nsproxy->ima_ns;
+ get_ima_ns(ns);
+ }
+ task_unlock(task);
+
+ return ns ? &ns->ns : NULL;
+}
+
+static void imans_put(struct ns_common *ns)
+{
+ put_ima_ns(to_ima_ns(ns));
+}
+
+static int imans_install(struct nsproxy *nsproxy, struct ns_common *new)
+{
+ struct ima_namespace *ns = to_ima_ns(new);
+
+ if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN) ||
+ !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
+ return -EPERM;
+
+ get_ima_ns(ns);
+ put_ima_ns(nsproxy->ima_ns);
+ nsproxy->ima_ns = ns;
+ return 0;
+}
+
+const struct proc_ns_operations imans_operations = {
+ .name = "ima",
+ .type = CLONE_NEWNS,
+ .get = imans_get,
+ .put = imans_put,
+ .install = imans_install,
+};
+
+struct ima_namespace init_ima_ns = {
+ .kref = KREF_INIT(2),
+ .user_ns = &init_user_ns,
+ .ns.inum = PROC_IMA_INIT_INO,
+#ifdef CONFIG_IMA_NS
+ .ns.ops = &imans_operations,
+#endif
+ .parent = NULL,
+};
+EXPORT_SYMBOL(init_ima_ns);
--
2.9.4
|
|
From: Mehmet K. <mka...@li...> - 2017-07-20 22:55:12
|
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. A lazy clean up of the rbtree that removes free'd inodes could be
implemented to reclaim the invalid entries.
Signed-off-by: Mehmet Kayaalp <mka...@li...>
---
include/linux/ima.h | 3 +
security/integrity/ima/ima.h | 16 ++++++
security/integrity/ima/ima_ns.c | 120 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 139 insertions(+)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 11e4841..3fdf56f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -111,6 +111,9 @@ struct ima_namespace {
struct user_namespace *user_ns;
struct ns_common ns;
struct ima_namespace *parent;
+ 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/ima.h b/security/integrity/ima/ima.h
index 8a8234a..5ab769a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -128,6 +128,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);
@@ -293,11 +301,19 @@ static inline int ima_read_xattr(struct dentry *dentry,
#ifdef CONFIG_IMA_NS
int ima_ns_init(void);
+struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
+ struct inode *inode);
#else
static inline int ima_ns_init(void)
{
return 0;
}
+
+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 */
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 383217b..5ec5a4b 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -20,6 +20,9 @@ static void get_ima_ns(struct ima_namespace *ns);
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);
return 0;
}
@@ -98,10 +101,24 @@ struct ima_namespace *copy_ima(unsigned long flags,
return new_ns;
}
+static 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);
+}
+
static void destroy_ima_ns(struct ima_namespace *ns)
{
put_user_ns(ns->user_ns);
ns_free_inum(&ns->ns);
+ free_ns_status_cache(ns);
kfree(ns);
}
@@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
.parent = NULL,
};
EXPORT_SYMBOL(init_ima_ns);
+
+/*
+ * __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->i_ino > status->i_ino)
+ 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;
+}
+
+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. In the future
+ * we might want to add support for lazily walking the rbtree
+ * to clean it up.
+ */
+ 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.9.4
|
|
From: Stefan B. <st...@li...> - 2017-08-11 15:01:09
|
On 07/20/2017 06:50 PM, Mehmet Kayaalp wrote:
> 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. A lazy clean up of the rbtree that removes free'd inodes could be
> implemented to reclaim the invalid entries.
>
> Signed-off-by: Mehmet Kayaalp <mka...@li...>
> ---
> include/linux/ima.h | 3 +
> security/integrity/ima/ima.h | 16 ++++++
> security/integrity/ima/ima_ns.c | 120 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 139 insertions(+)
>
>
> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> .parent = NULL,
> };
> EXPORT_SYMBOL(init_ima_ns);
> +
> +/*
> + * __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->i_ino > status->i_ino)
> + n = n->rb_right;
Above you are comparing with the inode ptr, here with i_ino. Why can you
not compare with inode both times. Also the insertion only seems to
consider the inode ptr.
Stefan
|
|
From: Mehmet K. <mka...@li...> - 2017-07-20 22:55:57
|
The iint cache stores whether the file is measured, appraised, audited
etc. This patch moves the IMA_AUDITED flag into the per-namespace
ns_status, enabling IMA audit mechanism to audit the same file each time
it is accessed in a new namespace.
The ns_status is not looked up if the CONFIG_IMA_NS is disabled or if
none of the IMA_NS_STATUS_ACTIONS (currently only IMA_AUDIT) are enabled.
Read and write operations on the iint flags is replaced with function
calls. For reading, iint_flags() returns the bitwise AND of iint->flags
and ns_status->flags. The ns_status flags are masked with
IMA_NS_STATUS_FLAGS (currently only IMA_AUDITED). Similarly
set_iint_flags() only writes the masked portion to the ns_status flags,
while the iint flags is set as before. The ns_status parameter added to
ima_audit_measurement() is used with the above functions to query and
set the ns_status flags.
Signed-off-by: Mehmet Kayaalp <mka...@li...>
---
init/Kconfig | 4 +++-
security/integrity/ima/ima.h | 24 +++++++++++++++++++++++-
security/integrity/ima/ima_api.c | 8 +++++---
security/integrity/ima/ima_main.c | 15 ++++++++++++---
security/integrity/ima/ima_ns.c | 21 +++++++++++++++++++++
5 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/init/Kconfig b/init/Kconfig
index 339f84d..6bfc579 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1294,7 +1294,9 @@ config IMA_NS
help
Allow the creation of IMA namespaces for each mount namespace.
Namespaced IMA data enables having IMA features work separately
- for each mount namespace.
+ for each mount namespace. Currently, only the audit status flags
+ are stored in the namespace, which allows the same file to be
+ audited each time it is accessed in a new namespace.
endif # NAMESPACES
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 5ab769a..78921b7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -210,7 +210,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, int pcr);
void ima_audit_measurement(struct integrity_iint_cache *iint,
- const unsigned char *filename);
+ const unsigned char *filename,
+ struct ns_status *status);
int ima_alloc_init_template(struct ima_event_data *event_data,
struct ima_template_entry **entry);
int ima_store_template(struct ima_template_entry *entry, int violation,
@@ -299,10 +300,17 @@ static inline int ima_read_xattr(struct dentry *dentry,
#endif /* CONFIG_IMA_APPRAISE */
+#define IMA_NS_STATUS_ACTIONS IMA_AUDIT
+#define IMA_NS_STATUS_FLAGS IMA_AUDITED
+
#ifdef CONFIG_IMA_NS
int ima_ns_init(void);
struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
struct inode *inode);
+unsigned long iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status);
+unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status, unsigned long flags);
#else
static inline int ima_ns_init(void)
{
@@ -314,6 +322,20 @@ static inline struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
{
return NULL;
}
+
+static inline unsigned long iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status)
+{
+ return iint->flags;
+}
+
+static inline unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status,
+ unsigned long flags)
+{
+ iint->flags = flags;
+ return flags;
+}
#endif /* CONFIG_IMA_NS */
/* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c2edba8..4a77072 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -287,15 +287,17 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
}
void ima_audit_measurement(struct integrity_iint_cache *iint,
- const unsigned char *filename)
+ const unsigned char *filename,
+ struct ns_status *status)
{
struct audit_buffer *ab;
char hash[(iint->ima_hash->length * 2) + 1];
const char *algo_name = hash_algo_name[iint->ima_hash->algo];
char algo_hash[sizeof(hash) + strlen(algo_name) + 2];
int i;
+ unsigned long flags = iint_flags(iint, status);
- if (iint->flags & IMA_AUDITED)
+ if (flags & IMA_AUDITED)
return;
for (i = 0; i < iint->ima_hash->length; i++)
@@ -316,7 +318,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_task_info(ab, current);
audit_log_end(ab);
- iint->flags |= IMA_AUDITED;
+ set_iint_flags(iint, status, flags | IMA_AUDITED);
}
/*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb79..fb002f2 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -160,6 +160,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
{
struct inode *inode = file_inode(file);
struct integrity_iint_cache *iint = NULL;
+ struct ns_status *status = NULL;
struct ima_template_desc *template_desc;
char *pathbuf = NULL;
char filename[NAME_MAX];
@@ -170,6 +171,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
int xattr_len = 0;
bool violation_check;
enum hash_algo hash_algo;
+ unsigned long flags;
if (!ima_policy_flag || !S_ISREG(inode->i_mode))
return 0;
@@ -196,6 +198,12 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
iint = integrity_inode_get(inode);
if (!iint)
goto out;
+
+ if (action & IMA_NS_STATUS_ACTIONS) {
+ status = ima_get_ns_status(get_current_ns(), inode);
+ if (IS_ERR(status))
+ goto out;
+ }
}
if (violation_check) {
@@ -211,9 +219,10 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
* (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
* IMA_AUDIT, IMA_AUDITED)
*/
- iint->flags |= action;
+ flags = iint_flags(iint, status);
+ flags = set_iint_flags(iint, status, flags | action);
action &= IMA_DO_MASK;
- action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
+ action &= ~((flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
/* If target pcr is already measured, unset IMA_MEASURE action */
if ((action & IMA_MEASURE) && (iint->measured_pcrs & (0x1 << pcr)))
@@ -251,7 +260,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
rc = ima_appraise_measurement(func, iint, file, pathname,
xattr_value, xattr_len, opened);
if (action & IMA_AUDIT)
- ima_audit_measurement(iint, pathname);
+ ima_audit_measurement(iint, pathname, status);
out_digsig:
if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) &&
diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
index 5ec5a4b..562a0812 100644
--- a/security/integrity/ima/ima_ns.c
+++ b/security/integrity/ima/ima_ns.c
@@ -301,3 +301,24 @@ struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
return status;
}
+
+#define IMA_NS_STATUS_ACTIONS IMA_AUDIT
+#define IMA_NS_STATUS_FLAGS IMA_AUDITED
+
+unsigned long iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status)
+{
+ if (!status)
+ return iint->flags;
+
+ return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS);
+}
+
+unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status, unsigned long flags)
+{
+ iint->flags = flags;
+ if (status)
+ status->flags = flags & IMA_NS_STATUS_FLAGS;
+ return flags;
+}
--
2.9.4
|
|
From: Tycho A. <ty...@do...> - 2017-08-01 17:45:30
|
Hi Mehmet,
On Thu, Jul 20, 2017 at 06:50:31PM -0400, Mehmet Kayaalp wrote:
> --- a/security/integrity/ima/ima_ns.c
> +++ b/security/integrity/ima/ima_ns.c
> @@ -301,3 +301,24 @@ struct ns_status *ima_get_ns_status(struct ima_namespace *ns,
>
> return status;
> }
> +
> +#define IMA_NS_STATUS_ACTIONS IMA_AUDIT
> +#define IMA_NS_STATUS_FLAGS IMA_AUDITED
> +
Seems like these are defined in ima.h above in the patch, and
re-defined here?
> +unsigned long iint_flags(struct integrity_iint_cache *iint,
> + struct ns_status *status)
> +{
> + if (!status)
> + return iint->flags;
> +
> + return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS);
Just to confirm, is there any situation where:
iint->flags & IMA_NS_STATUS_FLAGS != status->flags & IMA_NS_STATUS_FLAGS
? i.e. can this line just be:
return status->flags & IMA_NS_STATUS_FLAGS;
Tycho
> +}
> +
> +unsigned long set_iint_flags(struct integrity_iint_cache *iint,
> + struct ns_status *status, unsigned long flags)
> +{
> + iint->flags = flags;
> + if (status)
> + status->flags = flags & IMA_NS_STATUS_FLAGS;
> + return flags;
> +}
> --
> 2.9.4
>
|
|
From: Mehmet K. <mka...@li...> - 2017-07-20 22:57:08
|
From: Guilherme Magalhaes <gui...@hp...>
Extending audit measurement record with mount namespace id, file inode,
and device name. These fields uniquely identify a pathname considering
different mount namespaces. The file inode on a given device is unique
and these fields are required to identify a namespace id since this id
can be released and later reused by a different process.
Signed-off-by: Guilherme Magalhaes <gui...@hp...>
Changelog:
* Change the field name from "mnt_ns" to "ns_mnt"
Signed-off-by: Mehmet Kayaalp <mka...@li...>
---
security/integrity/ima/ima_api.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 4a77072..084b126 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -18,6 +18,7 @@
#include <linux/fs.h>
#include <linux/xattr.h>
#include <linux/evm.h>
+#include <linux/proc_ns.h>
#include "ima.h"
@@ -296,6 +297,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
char algo_hash[sizeof(hash) + strlen(algo_name) + 2];
int i;
unsigned long flags = iint_flags(iint, status);
+ struct ns_common *ns;
if (flags & IMA_AUDITED)
return;
@@ -314,6 +316,14 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_format(ab, " hash=");
snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
audit_log_untrustedstring(ab, algo_hash);
+ ns = mntns_operations.get(current);
+ if (!IS_ERR_OR_NULL(ns)) {
+ audit_log_format(ab, " ns_mnt=%u", ns->inum);
+ mntns_operations.put(ns);
+ }
+ audit_log_format(ab, " dev=");
+ audit_log_untrustedstring(ab, iint->inode->i_sb->s_id);
+ audit_log_format(ab, " ino=%lu", iint->inode->i_ino);
audit_log_task_info(ab, current);
audit_log_end(ab);
--
2.9.4
|
|
Re: [Linux-ima-devel] [RFC PATCH 5/5] ima: Add ns_mnt, dev,
ino fields to IMA audit measurement msgs
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-21 20:12:00
|
Mehmet,
if (flags & IMA_AUDITED)
return;
@@ -314,6 +316,14 @@ void ima_audit_measurement(struct integrity_iint_cache *iint,
audit_log_format(ab, " hash=");
snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
audit_log_untrustedstring(ab, algo_hash);
+ ns = mntns_operations.get(current);
As pointed out in other email, probably we need to get the IMA namespace id to add it to the record as 'ns_ima' below.
--
Guilherme
+ if (!IS_ERR_OR_NULL(ns)) {
+ audit_log_format(ab, " ns_mnt=%u", ns->inum);
+ mntns_operations.put(ns);
+ }
+ audit_log_format(ab, " dev=");
+ audit_log_untrustedstring(ab, iint->inode->i_sb->s_id);
+ audit_log_format(ab, " ino=%lu", iint->inode->i_ino);
audit_log_task_info(ab, current);
audit_log_end(ab);
--
2.9.4
|
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-21 19:44:48
|
Mehmet, > As a result, clone() and unshare() with CLONE_NEWNS results in a new > mount and a new IMA namespace, while setns() called with the fd of > /proc/*/ns/mnt would NOT have the same result. A second setns() > call with the fd /proc/*/ns/ima would be required. If I understood correctly, a given mount namespace is always paired with a given IMA namespace, but processes which setns() to a given mount namespace, would need an additional step to fix the pairing. That means when a process changes to a different mount namespace, the namespaced IMA flags are still the flags 'controlled' by the old mount namespace until the IMA namespace is fixed with a new setns(). It is also possible that a process points to two completely unrelated mount and IMA namespaces. This namespaces relation might confuse the audit log since a given mount namespace id might appear twice for the same file with no file change, or an expected entry may be not logged since the process changed to an IMA namespace where the inode namespaced flag was already IMA_AUDITED. Probably one side effect from that is in the audit log, which should be fixed replacing 'ns_mnt' with 'ns_ima' on the audit record. -- Guilherme -----Original Message----- From: own...@vg... [mailto:own...@vg...] On Behalf Of Mehmet Kayaalp Sent: quinta-feira, 20 de julho de 2017 19:50 To: ima-devel <lin...@li...> Cc: containers <con...@li...>; linux-kernel <lin...@vg...>; linux-security-module <lin...@vg...>; Tycho Andersen <ty...@do...>; Serge E . Hallyn <se...@ha...>; Yuqiong Sun <sun...@gm...>; David Safford <dav...@ge...>; Mehmet Kayaalp <mka...@cs...>; Stefan Berger <st...@li...>; Mehmet Kayaalp <mka...@li...> Subject: [RFC PATCH 0/5] ima: namespacing IMA audit messages This patch set implements an IMA namespace data structure that gets created alongside a mount namespace with CLONE_NEWNS, and lays down the foundation for namespacing the different aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal). The original PoC patches [1], created a new CLONE_NEWIMA flag to explicitly control when a new IMA namespace should be created. Based on comments, we elected to hang the IMA namepace off of existing namespaces, and the mount namespace made the most sense. However, we actually allocate a new namespace struct in nsproxy, allocate a new inum, and have an ima symlink in /proc/*/ns/, instead of adding a pointer from the mnt_namespace. As a result, clone() and unshare() with CLONE_NEWNS results in a new mount and a new IMA namespace, while setns() called with the fd of /proc/*/ns/mnt would NOT have the same result. A second setns() call with the fd /proc/*/ns/ima would be required. The first patch creates the ima_namespace data, while the second patch puts the iint->flags in the namespace. The third patch uses these flags for namespacing the IMA-audit messages, enabling the same file to be audited each time it is accessed in a new namespace. Rest of the patches are small fixes and improvements to the audit messages generated by IMA. Subsequent patch sets will namespace IMA-measurement and IMA-appraisal. [1] https://sourceforge.net/p/linux-ima/mailman/message/35939754/ Guilherme Magalhaes (1): ima: Add ns_mnt, dev, ino fields to IMA audit measurement msgs Mehmet Kayaalp (2): ima: Add ns_status for storing namespaced iint data ima: mamespace audit status flags Mimi Zohar (1): ima: differentiate auditing policy rules from "audit" actions Yuqiong Sun (1): ima: extend clone() with IMA namespace support fs/proc/namespaces.c | 3 + include/linux/ima.h | 40 +++++ include/linux/nsproxy.h | 1 + include/linux/proc_ns.h | 2 + include/uapi/linux/audit.h | 3 +- init/Kconfig | 10 ++ kernel/nsproxy.c | 15 ++ security/integrity/ima/Makefile | 1 + security/integrity/ima/ima.h | 49 +++++- security/integrity/ima/ima_api.c | 18 +- security/integrity/ima/ima_init.c | 4 + security/integrity/ima/ima_main.c | 15 +- security/integrity/ima/ima_ns.c | 324 ++++++++++++++++++++++++++++++++++++ security/integrity/ima/ima_policy.c | 2 +- 14 files changed, 478 insertions(+), 9 deletions(-) create mode 100644 security/integrity/ima/ima_ns.c -- 2.9.4 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to maj...@vg... More majordomo info at http://vger.kernel.org/majordomo-info.html |
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-21 19:46:08
|
Mehmet,
+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. In the future
+ * we might want to add support for lazily walking the rbtree
+ * to clean it up.
+ */
+ if (inode->i_ino == status->i_ino &&
+ inode->i_generation == status->i_generation)
As pointed in another email, probably we need to check the 'i_version' here to account for the file change case. when files change, we want to clear out the namespaced flags.
--
Guilherme
+ 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.9.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to maj...@vg... More majordomo info at http://vger.kernel.org/majordomo-info.html
|
|
From: Mehmet K. <mka...@li...> - 2017-07-24 19:27:17
|
> On Jul 21, 2017, at 3:45 PM, Magalhaes, Guilherme (Brazil R&D-CL) <gui...@hp...> wrote:
>
> Mehmet,
>
> +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. In the future
> + * we might want to add support for lazily walking the rbtree
> + * to clean it up.
> + */
> + if (inode->i_ino == status->i_ino &&
> + inode->i_generation == status->i_generation)
>
> As pointed in another email, probably we need to check the 'i_version' here to account for the file change case. when files change, we want to clear out the namespaced flags.
Sure, good catch.
Mehmet
|
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-21 20:30:19
|
Mehmet,
+#define IMA_NS_STATUS_ACTIONS IMA_AUDIT
+#define IMA_NS_STATUS_FLAGS IMA_AUDITED
+
+unsigned long iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status)
+{
+ if (!status)
+ return iint->flags;
+
+ return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS); }
I believe the first '&' should be replaced with '|', so you can return the consolidated 'flags | ns_flags'.
+
+unsigned long set_iint_flags(struct integrity_iint_cache *iint,
+ struct ns_status *status, unsigned long flags) {
+ iint->flags = flags;
I believe the global flags should consider if status is NULL, otherwise the namespaced flags such as IMA_AUDITED should not be set to the global flags otherwise this namespace flag IMA_AUDITED could be incorrectly injected into other namespaces.
--
Guilherme
+ if (status)
+ status->flags = flags & IMA_NS_STATUS_FLAGS;
+ return flags;
+}
--
2.9.4
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to maj...@vg... More majordomo info at http://vger.kernel.org/majordomo-info.html
|
|
From: Mehmet K. <mka...@li...> - 2017-07-24 19:43:23
|
> On Jul 21, 2017, at 4:29 PM, Magalhaes, Guilherme (Brazil R&D-CL) <gui...@hp...> wrote:
>
> Mehmet,
>
> +#define IMA_NS_STATUS_ACTIONS IMA_AUDIT
> +#define IMA_NS_STATUS_FLAGS IMA_AUDITED
> +
> +unsigned long iint_flags(struct integrity_iint_cache *iint,
> + struct ns_status *status)
> +{
> + if (!status)
> + return iint->flags;
> +
> + return iint->flags & (status->flags & IMA_NS_STATUS_FLAGS); }
>
> I believe the first '&' should be replaced with '|', so you can return the consolidated 'flags | ns_flags'.
Yes, it should be OR'ed.
> +
> +unsigned long set_iint_flags(struct integrity_iint_cache *iint,
> + struct ns_status *status, unsigned long flags) {
> + iint->flags = flags;
>
> I believe the global flags should consider if status is NULL, otherwise the namespaced flags such as IMA_AUDITED should not be set to the global flags otherwise this namespace flag IMA_AUDITED could be incorrectly injected into other namespaces.
I think we can apply a mask:
iint->flags & ~(IMA_NS_STATUS_FLAGS)
so that it is clear these flags are moved to ns_status.
Mehmet
|
|
From: Mehmet K. <mka...@li...> - 2017-07-24 19:26:01
|
> On Jul 21, 2017, at 3:44 PM, Magalhaes, Guilherme (Brazil R&D-CL) <gui...@hp...> wrote: > > Mehmet, > >> As a result, clone() and unshare() with CLONE_NEWNS results in a new >> mount and a new IMA namespace, while setns() called with the fd of >> /proc/*/ns/mnt would NOT have the same result. A second setns() >> call with the fd /proc/*/ns/ima would be required. > If I understood correctly, a given mount namespace is always paired with a given IMA namespace, but processes which setns() to a given mount namespace, would need an additional step to fix the pairing. That means when a process changes to a different mount namespace, the namespaced IMA flags are still the flags 'controlled' by the old mount namespace until the IMA namespace is fixed with a new setns(). It is also possible that a process points to two completely unrelated mount and IMA namespaces. Yes, we would prefer a solution that does not require two synced calls to setns. I was thinking of putting the nsproxy pointer in the nsfs inode->i_private. So that, in setns we can get to the ima_namespace. > This namespaces relation might confuse the audit log since a given mount namespace id might appear twice for the same file with no file change, or an expected entry may be not logged since the process changed to an IMA namespace where the inode namespaced flag was already IMA_AUDITED. > Probably one side effect from that is in the audit log, which should be fixed replacing 'ns_mnt' with 'ns_ima' on the audit record. Yes, we could have both, maybe? Mehmet |
|
From: Magalhaes, G. (B. R&D-CL) <gui...@hp...> - 2017-07-28 14:28:36
|
> -----Original Message----- > From: Mehmet Kayaalp [mailto:mka...@li...] > Sent: segunda-feira, 24 de julho de 2017 16:30 > To: Magalhaes, Guilherme (Brazil R&D-CL) <gui...@hp...> > Cc: ima-devel <lin...@li...> > Subject: Re: [RFC PATCH 0/5] ima: namespacing IMA audit messages > > > > On Jul 21, 2017, at 3:44 PM, Magalhaes, Guilherme (Brazil R&D-CL) > <gui...@hp...> wrote: > > > > Mehmet, > > > >> As a result, clone() and unshare() with CLONE_NEWNS results in a new > >> mount and a new IMA namespace, while setns() called with the fd of > >> /proc/*/ns/mnt would NOT have the same result. A second setns() > >> call with the fd /proc/*/ns/ima would be required. > > If I understood correctly, a given mount namespace is always paired with a > given IMA namespace, but processes which setns() to a given mount > namespace, would need an additional step to fix the pairing. That means > when a process changes to a different mount namespace, the namespaced > IMA flags are still the flags 'controlled' by the old mount namespace until the > IMA namespace is fixed with a new setns(). It is also possible that a process > points to two completely unrelated mount and IMA namespaces. > > Yes, we would prefer a solution that does not require two synced calls > to setns. I was thinking of putting the nsproxy pointer in the nsfs > inode->i_private. So that, in setns we can get to the ima_namespace. > > > This namespaces relation might confuse the audit log since a given mount > namespace id might appear twice for the same file with no file change, or an > expected entry may be not logged since the process changed to an IMA > namespace where the inode namespaced flag was already IMA_AUDITED. > > Probably one side effect from that is in the audit log, which should be fixed > replacing 'ns_mnt' with 'ns_ima' on the audit record. > > Yes, we could have both, maybe? Right, on this case, both might be needed. -- Guilherme > > Mehmet |
|
From: Serge E. H. <se...@ha...> - 2017-07-25 18:10:06
|
On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote: > From: Yuqiong Sun <su...@us...> > > Add new CONFIG_IMA_NS config option. Let clone() create a new IMA > namespace upon CLONE_NEWNS flag. Add ima_ns data structure in nsproxy. > ima_ns is allocated and freed upon IMA namespace creation and exit. > Currently, the ima_ns contains no useful IMA data but only a dummy > interface. This patch creates the framework for namespacing the different > aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal). > > Signed-off-by: Yuqiong Sun <su...@us...> > > Changelog: > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag Hi, So this means that every mount namespace clone will clone a new IMA namespace. Is that really ok? |
|
From: James B. <Jam...@Ha...> - 2017-07-25 19:06:31
|
On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote: > > > > From: Yuqiong Sun <su...@us...> > > > > Add new CONFIG_IMA_NS config option. Let clone() create a new IMA > > namespace upon CLONE_NEWNS flag. Add ima_ns data structure in > > nsproxy. > > ima_ns is allocated and freed upon IMA namespace creation and exit. > > Currently, the ima_ns contains no useful IMA data but only a dummy > > interface. This patch creates the framework for namespacing the > > different > > aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal). > > > > Signed-off-by: Yuqiong Sun <su...@us...> > > > > Changelog: > > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag > > Hi, > > So this means that every mount namespace clone will clone a new IMA > namespace. Is that really ok? Based on what: space concerns (struct ima_ns is reasonably small)? or whether tying it to the mount namespace is the correct thing to do. On the latter, it does seem that this should be a property of either the mount or user ns rather than its own separate ns. I could see a use where even a container might want multiple ima keyrings within the container (say containerised apache service with multiple tenants), so instinct tells me that mount ns is the correct granularity for this. James |
|
From: Serge E. H. <se...@ha...> - 2017-07-25 19:04:07
|
On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote: > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > > On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote: > > > > > > From: Yuqiong Sun <su...@us...> > > > > > > Add new CONFIG_IMA_NS config option. Let clone() create a new IMA > > > namespace upon CLONE_NEWNS flag. Add ima_ns data structure in > > > nsproxy. > > > ima_ns is allocated and freed upon IMA namespace creation and exit. > > > Currently, the ima_ns contains no useful IMA data but only a dummy > > > interface. This patch creates the framework for namespacing the > > > different > > > aspects of IMA (eg. IMA-audit, IMA-measurement, IMA-appraisal). > > > > > > Signed-off-by: Yuqiong Sun <su...@us...> > > > > > > Changelog: > > > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag > > > > Hi, > > > > So this means that every mount namespace clone will clone a new IMA > > namespace. Is that really ok? > > Based on what: space concerns (struct ima_ns is reasonably small)? or > whether tying it to the mount namespace is the correct thing to do. On Mostly the latter. The other would be not so much space concerns as time concerns. Many things use new mounts namespaces, and we wouldn't want multiple IMA calls on all file accesses by all of those. > the latter, it does seem that this should be a property of either the > mount or user ns rather than its own separate ns. I could see a use > where even a container might want multiple ima keyrings within the > container (say containerised apache service with multiple tenants), so > instinct tells me that mount ns is the correct granularity for this. I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns as the trigger for requesting a new ima ns on the next clone(CLONE_NEWNS). |
|
From: James B. <Jam...@Ha...> - 2017-07-25 19:09:32
|
On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote: > > > > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > > > > > > On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote: > > > > > > > > > > > > From: Yuqiong Sun <su...@us...> > > > > > > > > Add new CONFIG_IMA_NS config option. Let clone() create a new > > > > IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure > > > > in nsproxy. ima_ns is allocated and freed upon IMA namespace > > > > creation and exit. Currently, the ima_ns contains no useful IMA > > > > data but only a dummy interface. This patch creates the > > > > framework for namespacing the different aspects of IMA (eg. > > > > IMA-audit, IMA-measurement, IMA-appraisal). > > > > > > > > Signed-off-by: Yuqiong Sun <su...@us...> > > > > > > > > Changelog: > > > > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag > > > > > > Hi, > > > > > > So this means that every mount namespace clone will clone a new > > > IMA namespace. Is that really ok? > > > > Based on what: space concerns (struct ima_ns is reasonably small)? > > or whether tying it to the mount namespace is the correct thing to > > do. On > > Mostly the latter. The other would be not so much space concerns as > time concerns. Many things use new mounts namespaces, and we > wouldn't want multiple IMA calls on all file accesses by all of > those. > > > > > the latter, it does seem that this should be a property of either > > the mount or user ns rather than its own separate ns. I could see > > a use where even a container might want multiple ima keyrings > > within the container (say containerised apache service with > > multiple tenants), so instinct tells me that mount ns is the > > correct granularity for this. > > I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns > as the trigger for requesting a new ima ns on the next > clone(CLONE_NEWNS). I could go with that, but what about the trigger being installing or updating the keyring? That's the only operation that needs namespace separation, so on mount ns clone, you get a pointer to the old ima_ns until you do something that requires a new key, which then triggers the copy of the namespace and installing it? James |
|
From: Serge E. H. <se...@ha...> - 2017-07-25 19:43:16
|
...
> +static 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);
> +}
> +
> static void destroy_ima_ns(struct ima_namespace *ns)
> {
> put_user_ns(ns->user_ns);
> ns_free_inum(&ns->ns);
> + free_ns_status_cache(ns);
> kfree(ns);
> }
>
> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> .parent = NULL,
> };
> EXPORT_SYMBOL(init_ima_ns);
> +
> +/*
> + * __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->i_ino > status->i_ino)
> + 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;
> +}
...
> +
> +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. In the future
> + * we might want to add support for lazily walking the rbtree
> + * to clean it up.
> + */
> + 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);
> + }
What prevents the status from being freed between the read_lock
in ima_ns_status_find() and the write_lock in the following line?
IIUC it's that ns is always current's ima_ns, which will pin the ns
and cause no statuses to be freed. But then the ns should probably
not be passed in here? Or a comment should say that ns must be
pinned?
Just trying to make sure I understand the locking.
> + 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.9.4
|
|
From: Mimi Z. <zo...@li...> - 2017-07-25 19:48:23
|
On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote: > On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote: > > On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote: > > > > > > On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote: > > > > > > > > On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote: > > > > > > > > > > > > > > > From: Yuqiong Sun <su...@us...> > > > > > > > > > > Add new CONFIG_IMA_NS config option. Let clone() create a new > > > > > IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure > > > > > in nsproxy. ima_ns is allocated and freed upon IMA namespace > > > > > creation and exit. Currently, the ima_ns contains no useful IMA > > > > > data but only a dummy interface. This patch creates the > > > > > framework for namespacing the different aspects of IMA (eg. > > > > > IMA-audit, IMA-measurement, IMA-appraisal). > > > > > > > > > > Signed-off-by: Yuqiong Sun <su...@us...> > > > > > > > > > > Changelog: > > > > > * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag > > > > > > > > Hi, > > > > > > > > So this means that every mount namespace clone will clone a new > > > > IMA namespace. Is that really ok? > > > > > > Based on what: space concerns (struct ima_ns is reasonably small)? > > > or whether tying it to the mount namespace is the correct thing to > > > do. On > > > > Mostly the latter. The other would be not so much space concerns as > > time concerns. Many things use new mounts namespaces, and we > > wouldn't want multiple IMA calls on all file accesses by all of > > those. > > > > > > > > the latter, it does seem that this should be a property of either > > > the mount or user ns rather than its own separate ns. I could see > > > a use where even a container might want multiple ima keyrings > > > within the container (say containerised apache service with > > > multiple tenants), so instinct tells me that mount ns is the > > > correct granularity for this. > > > > I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns > > as the trigger for requesting a new ima ns on the next > > clone(CLONE_NEWNS). > > I could go with that, but what about the trigger being installing or > updating the keyring? That's the only operation that needs namespace > separation, so on mount ns clone, you get a pointer to the old ima_ns > until you do something that requires a new key, which then triggers the > copy of the namespace and installing it? It isn't just the keyrings that need to be namespaced, but the measurement list and policy as well. IMA-measurement, IMA-appraisal and IMA-audit are all policy based. As soon as the namespace starts, measurements should be added to the namespace specific measurement list, not it's parent. Mimi |
|
From: Stefan B. <st...@li...> - 2017-07-25 20:11:45
|
On 07/25/2017 03:48 PM, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 12:08 -0700, James Bottomley wrote:
>> On Tue, 2017-07-25 at 14:04 -0500, Serge E. Hallyn wrote:
>>> On Tue, Jul 25, 2017 at 11:49:14AM -0700, James Bottomley wrote:
>>>> On Tue, 2017-07-25 at 12:53 -0500, Serge E. Hallyn wrote:
>>>>> On Thu, Jul 20, 2017 at 06:50:29PM -0400, Mehmet Kayaalp wrote:
>>>>>>
>>>>>> From: Yuqiong Sun <su...@us...>
>>>>>>
>>>>>> Add new CONFIG_IMA_NS config option. Let clone() create a new
>>>>>> IMA namespace upon CLONE_NEWNS flag. Add ima_ns data structure
>>>>>> in nsproxy. ima_ns is allocated and freed upon IMA namespace
>>>>>> creation and exit. Currently, the ima_ns contains no useful IMA
>>>>>> data but only a dummy interface. This patch creates the
>>>>>> framework for namespacing the different aspects of IMA (eg.
>>>>>> IMA-audit, IMA-measurement, IMA-appraisal).
>>>>>>
>>>>>> Signed-off-by: Yuqiong Sun <su...@us...>
>>>>>>
>>>>>> Changelog:
>>>>>> * Use CLONE_NEWNS instead of a new CLONE_NEWIMA flag
>>>>> Hi,
>>>>>
>>>>> So this means that every mount namespace clone will clone a new
>>>>> IMA namespace. Is that really ok?
>>>> Based on what: space concerns (struct ima_ns is reasonably small)?
>>>> or whether tying it to the mount namespace is the correct thing to
>>>> do. On
>>> Mostly the latter. The other would be not so much space concerns as
>>> time concerns. Many things use new mounts namespaces, and we
>>> wouldn't want multiple IMA calls on all file accesses by all of
>>> those.
>>>
>>>> the latter, it does seem that this should be a property of either
>>>> the mount or user ns rather than its own separate ns. I could see
>>>> a use where even a container might want multiple ima keyrings
>>>> within the container (say containerised apache service with
>>>> multiple tenants), so instinct tells me that mount ns is the
>>>> correct granularity for this.
>>> I wonder whether we could use echo 1 > /sys/kernel/security/ima/newns
>>> as the trigger for requesting a new ima ns on the next
>>> clone(CLONE_NEWNS).
>> I could go with that, but what about the trigger being installing or
>> updating the keyring? That's the only operation that needs namespace
>> separation, so on mount ns clone, you get a pointer to the old ima_ns
>> until you do something that requires a new key, which then triggers the
>> copy of the namespace and installing it?
> It isn't just the keyrings that need to be namespaced, but the
> measurement list and policy as well.
>
> IMA-measurement, IMA-appraisal and IMA-audit are all policy based.
>
> As soon as the namespace starts, measurements should be added to the
> namespace specific measurement list, not it's parent.
IMA is about measuring things, logging what was executed, and finally
someone looking at the measurement log and detecting 'things'. So at
least one attack that needs to be prevented is a malicious person
opening an IMA namespace, executing something malicious, and not leaving
any trace on the host because all the logs went into the measurement
list of the IMA namespace, which disappeared. That said, I am wondering
whether there has to be a minimum set of namespaces (PID, UTS)
providing enough 'isolation' that someone may actually open an IMA
namespace and run their code. To avoid leaving no traces one could argue
to implement recursive logging, so something that is logged inside the
namespace will be detected in all parent containers up to the
init_ima_ns (host) because it's logged (and TPM extended) there as well.
The challenge with that is that logging costs memory and that can be
abused as well until the machine needs a reboot... I guess the solution
could be requesting an IMA namespace in one way or another but requiring
several other namespace flags in the clone() to actually 'get' it.
Jumping namespaces with setns() may have to be restricted as well once
there is an IMA namespace.
Stefan
>
> Mimi
>
|
|
From: Mimi Z. <zo...@li...> - 2017-07-25 20:15:48
|
On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
> ...
> > +static 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);
> > +}
> > +
> > static void destroy_ima_ns(struct ima_namespace *ns)
> > {
> > put_user_ns(ns->user_ns);
> > ns_free_inum(&ns->ns);
> > + free_ns_status_cache(ns);
> > kfree(ns);
> > }
> >
> > @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> > .parent = NULL,
> > };
> > EXPORT_SYMBOL(init_ima_ns);
> > +
> > +/*
> > + * __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->i_ino > status->i_ino)
> > + 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;
> > +}
> ...
> > +
> > +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. In the future
> > + * we might want to add support for lazily walking the rbtree
> > + * to clean it up.
> > + */
> > + 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);
> > + }
>
> What prevents the status from being freed between the read_lock
> in ima_ns_status_find() and the write_lock in the following line?
>
> IIUC it's that ns is always current's ima_ns, which will pin the ns
> and cause no statuses to be freed. But then the ns should probably
> not be passed in here? Or a comment should say that ns must be
> pinned?
>
> Just trying to make sure I understand the locking.
iint's are only freed after the last reference to the inode is deleted
in __fput(). Refer to ima_file_free(). ns_status is a bit different
in that they are freed on namespace cleanup.
Mimi
> > + 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.9.
>
|
|
From: Stefan B. <st...@li...> - 2017-07-25 20:25:31
|
On 07/25/2017 04:15 PM, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
>> ...
>>> +static 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);
>>> +}
>>> +
>>> static void destroy_ima_ns(struct ima_namespace *ns)
>>> {
>>> put_user_ns(ns->user_ns);
>>> ns_free_inum(&ns->ns);
>>> + free_ns_status_cache(ns);
>>> kfree(ns);
>>> }
>>>
>>> @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
>>> .parent = NULL,
>>> };
>>> EXPORT_SYMBOL(init_ima_ns);
>>> +
>>> +/*
>>> + * __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->i_ino > status->i_ino)
>>> + 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;
>>> +}
>> ...
>>> +
>>> +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. In the future
>>> + * we might want to add support for lazily walking the rbtree
>>> + * to clean it up.
>>> + */
>>> + 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);
>>> + }
>> What prevents the status from being freed between the read_lock
>> in ima_ns_status_find() and the write_lock in the following line?
>>
>> IIUC it's that ns is always current's ima_ns, which will pin the ns
>> and cause no statuses to be freed. But then the ns should probably
>> not be passed in here? Or a comment should say that ns must be
>> pinned?
>>
>> Just trying to make sure I understand the locking.
> iint's are only freed after the last reference to the inode is deleted
> in __fput(). Refer to ima_file_free(). ns_status is a bit different
> in that they are freed on namespace cleanup.
It should be possible to move the write_lock() above the
status = ima_ns_status_find(ns, inode);
and instead call __ima_ns_status_find() with the write_lock() held.
Stefan
>
> Mimi
>
>>> + 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.9.
|
|
From: Serge E. H. <se...@ha...> - 2017-07-25 20:49:41
|
On Tue, Jul 25, 2017 at 04:15:25PM -0400, Mimi Zohar wrote:
> On Tue, 2017-07-25 at 14:43 -0500, Serge E. Hallyn wrote:
> > ...
> > > +static 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);
> > > +}
> > > +
> > > static void destroy_ima_ns(struct ima_namespace *ns)
> > > {
> > > put_user_ns(ns->user_ns);
> > > ns_free_inum(&ns->ns);
> > > + free_ns_status_cache(ns);
> > > kfree(ns);
> > > }
> > >
> > > @@ -181,3 +198,106 @@ struct ima_namespace init_ima_ns = {
> > > .parent = NULL,
> > > };
> > > EXPORT_SYMBOL(init_ima_ns);
> > > +
> > > +/*
> > > + * __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->i_ino > status->i_ino)
> > > + 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;
> > > +}
> > ...
> > > +
> > > +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. In the future
> > > + * we might want to add support for lazily walking the rbtree
> > > + * to clean it up.
> > > + */
> > > + 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);
> > > + }
> >
> > What prevents the status from being freed between the read_lock
> > in ima_ns_status_find() and the write_lock in the following line?
> >
> > IIUC it's that ns is always current's ima_ns, which will pin the ns
> > and cause no statuses to be freed. But then the ns should probably
> > not be passed in here? Or a comment should say that ns must be
> > pinned?
> >
> > Just trying to make sure I understand the locking.
>
> iint's are only freed after the last reference to the inode is deleted
> in __fput(). Refer to ima_file_free(). ns_status is a bit different
> in that they are freed on namespace cleanup.
Ok, thanks - that sounds ok then.
|
Thanks for helping keep SourceForge clean.
X