Re: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related variables into ima_namespace

[Christian Brauner]

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?