Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related variables into ima_namespace
From: Stefan Berger
Date: Thu Dec 09 2021 - 15:43:04 EST
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);