Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related variables into ima_namespace
From: Christian Brauner <hidden>
Date: 2021-12-09 19:11:26
Also in:
linux-security-module, lkml
Subsystem:
extended verification module (evm), integrity measurement architecture (ima), security subsystem, the rest · Maintainers:
Mimi Zohar, Roberto Sassu, Dmitry Kasatkin, Paul Moore, James Morris, "Serge E. Hallyn", Linus Torvalds
On Wed, Dec 08, 2021 at 05:18:15PM -0500, Stefan Berger wrote:
quoted hunk ↗ jump to hunk
Move the ima_write_mutex, ima_fs_flag, and valid_policy variables into ima_namespace. This way each IMA namespace can set those variables independently. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- include/linux/ima.h | 5 ++++ security/integrity/ima/ima_fs.c | 32 +++++++++++------------- security/integrity/ima/ima_init_ima_ns.c | 4 +++ 3 files changed, 23 insertions(+), 18 deletions(-)diff --git a/include/linux/ima.h b/include/linux/ima.h index 2ce801bfc449..3aaf6e806db4 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h@@ -261,6 +261,11 @@ struct ima_namespace { struct ima_h_table ima_htable; struct list_head ima_measurements; unsigned long binary_runtime_size; + + /* IMA's filesystem */ + struct mutex ima_write_mutex; + unsigned long ima_fs_flags; + int valid_policy; }; extern struct ima_namespace init_ima_ns;diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 38b1c26479b3..0e582ceecc7f 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c@@ -25,8 +25,6 @@ #include "ima.h" -static DEFINE_MUTEX(ima_write_mutex); - bool ima_canonical_fmt; static int __init default_canonical_fmt_setup(char *str) {@@ -37,8 +35,6 @@ static int __init default_canonical_fmt_setup(char *str) } __setup("ima_canonical_fmt", default_canonical_fmt_setup); -static int valid_policy = 1; - static ssize_t ima_show_htable_value(char __user *buf, size_t count, loff_t *ppos, atomic_long_t *val) {@@ -339,7 +335,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf, goto out; } - result = mutex_lock_interruptible(&ima_write_mutex); + result = mutex_lock_interruptible(&ns->ima_write_mutex); if (result < 0) goto out_free;@@ -354,12 +350,12 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf, } else { result = ima_parse_add_rule(ns, data); } - mutex_unlock(&ima_write_mutex); + mutex_unlock(&ns->ima_write_mutex); out_free: kfree(data); out: if (result < 0) - valid_policy = 0; + ns->valid_policy = 0; return result; }@@ -376,8 +372,6 @@ enum ima_fs_flags { IMA_FS_BUSY, }; -static unsigned long ima_fs_flags; - #ifdef CONFIG_IMA_READ_POLICY static const struct seq_operations ima_policy_seqops = { .start = ima_policy_start,@@ -392,6 +386,8 @@ static const struct seq_operations ima_policy_seqops = { */ static int ima_open_policy(struct inode *inode, struct file *filp) { + struct ima_namespace *ns = get_current_ns(); +
I'm a bit confused here. In all those callbacks: .open = ima_open_policy, .write = ima_write_policy, .release = ima_release_policy, you're calling get_current_ns() at the top of it. What guarantees that the same ima_namespace is returned here? What if the fd is sent to someone who is in a different user namespace and the write to that file? Maybe I'm just confused but wouldn't you want something like this? From 1f03dc427c583d5e9ebc9ebe9de77c3c535bbebe Mon Sep 17 00:00:00 2001 From: Christian Brauner <redacted> Date: Thu, 9 Dec 2021 20:07:02 +0100 Subject: [PATCH] !!!! HERE BE DRAGONS - UNTESTED !!!! --- security/integrity/ima/ima_fs.c | 43 +++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 583462b29cb5..d5b302b925b8 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c@@ -317,10 +317,14 @@ static ssize_t ima_read_policy(char *path) static ssize_t ima_write_policy(struct file *file, const char __user *buf, size_t datalen, loff_t *ppos) { - struct ima_namespace *ns = get_current_ns(); + struct ima_namespace *ns; + struct user_namespace *user_ns; char *data; ssize_t result; + user_ns = ima_filp_private(filp); + ns = user_ns->ima_ns + if (datalen >= PAGE_SIZE) datalen = PAGE_SIZE - 1;
@@ -373,26 +377,51 @@ static const struct seq_operations ima_policy_seqops = { }; #endif +static struct user_namespace *ima_filp_private(struct file *filp) +{ + if (!(filp->f_flags & O_WRONLY)) { +#ifdef CONFIG_IMA_READ_POLICY + struct seq_file *seq; + + seq = filp->private_data; + return seq->private; +#endif + } + return filp->private_data; +} + /* * ima_open_policy: sequentialize access to the policy file */ static int ima_open_policy(struct inode *inode, struct file *filp) { - struct ima_namespace *ns = get_current_ns(); + struct user_namespace *user_ns = current_user_ns(); + struct ima_namespace *ns = user_ns->ima_ns; if (!(filp->f_flags & O_WRONLY)) { #ifndef CONFIG_IMA_READ_POLICY return -EACCES; #else + int err; + struct seq_file *seq; + if ((filp->f_flags & O_ACCMODE) != O_RDONLY) return -EACCES; - if (!mac_admin_ns_capable(ima_user_ns(ns))) + if (!mac_admin_ns_capable(user_ns)) return -EPERM; - return seq_open(filp, &ima_policy_seqops); + err = seq_open(filp, &ima_policy_seqops); + if (err) + return err; + + seq = filp->private_data; + seq->private = user_ns; + return 0; #endif } if (test_and_set_bit(IMA_FS_BUSY, &ns->ima_fs_flags)) return -EBUSY; + + filp->private_data = user_ns; return 0; }
@@ -405,9 +434,13 @@ static int ima_open_policy(struct inode *inode, struct file *filp) */ static int ima_release_policy(struct inode *inode, struct file *file) { - struct ima_namespace *ns = get_current_ns(); + struct ima_namespace *ns; + struct user_namespace *user_ns; const char *cause = ns->valid_policy ? "completed" : "failed"; + user_ns = ima_filp_private(filp); + ns = user_ns->ima_ns + if ((file->f_flags & O_ACCMODE) == O_RDONLY) return seq_release(inode, file);
--
2.30.2