On 12/9/21 14:11, Christian Brauner wrote:
On Wed, Dec 08, 2021 at 05:18:15PM -0500, Stefan Berger wrote: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@xxxxxxxxxxxxx> --- 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?
I hadn't thought about inheritance or passing fds. But yes. I will adopt your patch and extend all the files to tie them to the user namespace they are opened in...
Thanks.
From 1f03dc427c583d5e9ebc9ebe9de77c3c535bbebe Mon Sep 17 00:00:00 2001 From: Christian Brauner <christian.brauner@xxxxxxxxxx> 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);
