LinuxLists.cc - [PATCH v5 00/16] ima: Namespace IMA with audit support in IMA-ns

[permalink] [raw]

Subject: [PATCH v5 15/16] ima: Move dentries into ima_namespace

Move the dentries into the ima_namespace for reuse by virtualized
SecurityFS. Implement function freeing the dentries in order of
files and symlinks before directories.

Signed-off-by: Stefan Berger <[email protected]>
---
include/linux/ima.h | 13 ++++++
security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++---------------
2 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 3aaf6e806db4..4dd64e318b15 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -220,6 +220,17 @@ struct ima_h_table {
struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
};

+enum {
+ IMAFS_DENTRY_DIR = 0,
+ IMAFS_DENTRY_SYMLINK,
+ IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS,
+ IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS,
+ IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT,
+ IMAFS_DENTRY_VIOLATIONS,
+ IMAFS_DENTRY_IMA_POLICY,
+ IMAFS_DENTRY_LAST
+};
+
struct ima_namespace {
struct kref kref;
struct user_namespace *user_ns;
@@ -266,6 +277,8 @@ struct ima_namespace {
struct mutex ima_write_mutex;
unsigned long ima_fs_flags;
int valid_policy;
+
+ struct dentry *dentry[IMAFS_DENTRY_LAST];
};

extern struct ima_namespace init_ima_ns;
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index a749a3e79304..3810d11fb463 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -360,14 +360,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
return result;
}

-static struct dentry *ima_dir;
-static struct dentry *ima_symlink;
-static struct dentry *binary_runtime_measurements;
-static struct dentry *ascii_runtime_measurements;
-static struct dentry *runtime_measurements_count;
-static struct dentry *violations;
-static struct dentry *ima_policy;
-
enum ima_fs_flags {
IMA_FS_BUSY,
};
@@ -437,8 +429,8 @@ static int ima_release_policy(struct inode *inode, struct file *file)

ima_update_policy(ns);
#if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
- securityfs_remove(ima_policy);
- ima_policy = NULL;
+ securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
+ ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
#elif defined(CONFIG_IMA_WRITE_POLICY)
clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
#elif defined(CONFIG_IMA_READ_POLICY)
@@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = {
.llseek = generic_file_llseek,
};

-int __init ima_fs_init(void)
+static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
{
- ima_dir = securityfs_create_dir("ima", integrity_dir);
- if (IS_ERR(ima_dir))
+ int i;
+
+ for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--)
+ securityfs_remove(ns->dentry[i]);
+
+ memset(ns->dentry, 0, sizeof(ns->dentry));
+}
+
+static int __init ima_securityfs_init(struct user_namespace *user_ns)
+{
+ struct ima_namespace *ns = user_ns->ima_ns;
+ struct dentry *ima_dir;
+
+ ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR]))
return -1;
+ ima_dir = ns->dentry[IMAFS_DENTRY_DIR];

- ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
- NULL);
- if (IS_ERR(ima_symlink))
+ ns->dentry[IMAFS_DENTRY_SYMLINK] =
+ securityfs_create_symlink("ima", NULL, "integrity/ima", NULL);
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK]))
goto out;

- binary_runtime_measurements =
+ ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] =
securityfs_create_file("binary_runtime_measurements",
S_IRUSR | S_IRGRP, ima_dir, NULL,
&ima_measurements_ops);
- if (IS_ERR(binary_runtime_measurements))
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS]))
goto out;

- ascii_runtime_measurements =
+ ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] =
securityfs_create_file("ascii_runtime_measurements",
S_IRUSR | S_IRGRP, ima_dir, NULL,
&ima_ascii_measurements_ops);
- if (IS_ERR(ascii_runtime_measurements))
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS]))
goto out;

- runtime_measurements_count =
+ ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] =
securityfs_create_file("runtime_measurements_count",
S_IRUSR | S_IRGRP, ima_dir, NULL,
&ima_measurements_count_ops);
- if (IS_ERR(runtime_measurements_count))
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT]))
goto out;

- violations =
+ ns->dentry[IMAFS_DENTRY_VIOLATIONS] =
securityfs_create_file("violations", S_IRUSR | S_IRGRP,
ima_dir, NULL, &ima_htable_violations_ops);
- if (IS_ERR(violations))
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS]))
goto out;

- ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
+ ns->dentry[IMAFS_DENTRY_IMA_POLICY] =
+ securityfs_create_file("policy", POLICY_FILE_FLAGS,
ima_dir, NULL,
&ima_measure_policy_ops);
- if (IS_ERR(ima_policy))
+ if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY]))
goto out;

return 0;
out:
- securityfs_remove(violations);
- securityfs_remove(runtime_measurements_count);
- securityfs_remove(ascii_runtime_measurements);
- securityfs_remove(binary_runtime_measurements);
- securityfs_remove(ima_symlink);
- securityfs_remove(ima_dir);
- securityfs_remove(ima_policy);
+ ima_fs_ns_free_dentries(ns);
return -1;
}
+
+int __init ima_fs_init(void)
+{
+ return ima_securityfs_init(&init_user_ns);
+}
--
2.31.1

2021-12-09 14:34:45

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> Move the dentries into the ima_namespace for reuse by virtualized
> SecurityFS. Implement function freeing the dentries in order of
> files and symlinks before directories.
>
> Signed-off-by: Stefan Berger <[email protected]>
> ---

This doesn't work as implemented, I think.

What I would have preferred and what I tried to explain in the earlier
review was:
Keep the dentry stashing global since it is only needed for init_ima_ns.
Then struct ima_namespace becomes way smaller and simpler.
If you do that then it makes sense to remove the additional dget() in
securityfs_create_dentry() for non-init_ima_ns.
Then you can rely on auto-cleanup in .kill_sb() or on
ima_securityfs_init() failure and you only need to call
ima_fs_ns_free_dentries() if ns != init_ima_ns.

IIuc, it seems you're currently doing one dput() too many since you're
calling securityfs_remove() in the error path for non-init_ima_ns which
relies on the previous increased dget() which we removed.

> include/linux/ima.h | 13 ++++++
> security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++---------------
> 2 files changed, 52 insertions(+), 33 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 3aaf6e806db4..4dd64e318b15 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -220,6 +220,17 @@ struct ima_h_table {
> struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
> };
>
> +enum {
> + IMAFS_DENTRY_DIR = 0,
> + IMAFS_DENTRY_SYMLINK,
> + IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS,
> + IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS,
> + IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT,
> + IMAFS_DENTRY_VIOLATIONS,
> + IMAFS_DENTRY_IMA_POLICY,
> + IMAFS_DENTRY_LAST
> +};
> +
> struct ima_namespace {
> struct kref kref;
> struct user_namespace *user_ns;
> @@ -266,6 +277,8 @@ struct ima_namespace {
> struct mutex ima_write_mutex;
> unsigned long ima_fs_flags;
> int valid_policy;
> +
> + struct dentry *dentry[IMAFS_DENTRY_LAST];
> };
>
> extern struct ima_namespace init_ima_ns;
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a749a3e79304..3810d11fb463 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -360,14 +360,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
> return result;
> }
>
> -static struct dentry *ima_dir;
> -static struct dentry *ima_symlink;
> -static struct dentry *binary_runtime_measurements;
> -static struct dentry *ascii_runtime_measurements;
> -static struct dentry *runtime_measurements_count;
> -static struct dentry *violations;
> -static struct dentry *ima_policy;
> -
> enum ima_fs_flags {
> IMA_FS_BUSY,
> };
> @@ -437,8 +429,8 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>
> ima_update_policy(ns);
> #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
> - securityfs_remove(ima_policy);
> - ima_policy = NULL;
> + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
> + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
> #elif defined(CONFIG_IMA_WRITE_POLICY)
> clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
> #elif defined(CONFIG_IMA_READ_POLICY)
> @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = {
> .llseek = generic_file_llseek,
> };
>
> -int __init ima_fs_init(void)
> +static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
> {
> - ima_dir = securityfs_create_dir("ima", integrity_dir);
> - if (IS_ERR(ima_dir))
> + int i;
> +
> + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--)
> + securityfs_remove(ns->dentry[i]);
> +
> + memset(ns->dentry, 0, sizeof(ns->dentry));
> +}
> +
> +static int __init ima_securityfs_init(struct user_namespace *user_ns)
> +{
> + struct ima_namespace *ns = user_ns->ima_ns;
> + struct dentry *ima_dir;
> +
> + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir);
> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR]))
> return -1;
> + ima_dir = ns->dentry[IMAFS_DENTRY_DIR];
>
> - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
> - NULL);
> - if (IS_ERR(ima_symlink))
> + ns->dentry[IMAFS_DENTRY_SYMLINK] =
> + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL);
> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK]))
> goto out;
>
> - binary_runtime_measurements =
> + ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] =
> securityfs_create_file("binary_runtime_measurements",
> S_IRUSR | S_IRGRP, ima_dir, NULL,
> &ima_measurements_ops);
> - if (IS_ERR(binary_runtime_measurements))
> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS]))
> goto out;
>
> - ascii_runtime_measurements =
> + ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] =
> securityfs_create_file("ascii_runtime_measurements",
> S_IRUSR | S_IRGRP, ima_dir, NULL,
> &ima_ascii_measurements_ops);
> - if (IS_ERR(ascii_runtime_measurements))
> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS]))
> goto out;
>
> - runtime_measurements_count =
> + ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] =
> securityfs_create_file("runtime_measurements_count",
> S_IRUSR | S_IRGRP, ima_dir, NULL,
> &ima_measurements_count_ops);
> - if (IS_ERR(runtime_measurements_count))
> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT]))
> goto out;
>
> - violations =
> + ns->dentry[IMAFS_DENTRY_VIOLATIONS] =
> securityfs_create_file("violations", S_IRUSR | S_IRGRP,
> ima_dir, NULL, &ima_htable_violations_ops);
> - if (IS_ERR(violations))
> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS]))
> goto out;
>
> - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
> + ns->dentry[IMAFS_DENTRY_IMA_POLICY] =
> + securityfs_create_file("policy", POLICY_FILE_FLAGS,
> ima_dir, NULL,
> &ima_measure_policy_ops);
> - if (IS_ERR(ima_policy))
> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY]))
> goto out;
>
> return 0;
> out:
> - securityfs_remove(violations);
> - securityfs_remove(runtime_measurements_count);
> - securityfs_remove(ascii_runtime_measurements);
> - securityfs_remove(binary_runtime_measurements);
> - securityfs_remove(ima_symlink);
> - securityfs_remove(ima_dir);
> - securityfs_remove(ima_policy);
> + ima_fs_ns_free_dentries(ns);
> return -1;
> }
> +
> +int __init ima_fs_init(void)
> +{
> + return ima_securityfs_init(&init_user_ns);
> +}
> --
> 2.31.1
>
>

2021-12-09 14:38:03

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
> On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> > Move the dentries into the ima_namespace for reuse by virtualized
> > SecurityFS. Implement function freeing the dentries in order of
> > files and symlinks before directories.
> >
> > Signed-off-by: Stefan Berger <[email protected]>
> > ---
>
> This doesn't work as implemented, I think.
>
> What I would have preferred and what I tried to explain in the earlier
> review was:
> Keep the dentry stashing global since it is only needed for init_ima_ns.
> Then struct ima_namespace becomes way smaller and simpler.
> If you do that then it makes sense to remove the additional dget() in
> securityfs_create_dentry() for non-init_ima_ns.
> Then you can rely on auto-cleanup in .kill_sb() or on
> ima_securityfs_init() failure and you only need to call
> ima_fs_ns_free_dentries() if ns != init_ima_ns.
>
> IIuc, it seems you're currently doing one dput() too many since you're
> calling securityfs_remove() in the error path for non-init_ima_ns which
> relies on the previous increased dget() which we removed.

If you really want to move the dentry stashing into struct ima_namespace
even though it's really unnecessary then you may as well not care about
the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns)
call in .kill_sb(). But I really think not dragging dentry stashing into
struct ima_namespace is the correct way to go about this.

>
> > include/linux/ima.h | 13 ++++++
> > security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++---------------
> > 2 files changed, 52 insertions(+), 33 deletions(-)
> >
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 3aaf6e806db4..4dd64e318b15 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -220,6 +220,17 @@ struct ima_h_table {
> > struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
> > };
> >
> > +enum {
> > + IMAFS_DENTRY_DIR = 0,
> > + IMAFS_DENTRY_SYMLINK,
> > + IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS,
> > + IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS,
> > + IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT,
> > + IMAFS_DENTRY_VIOLATIONS,
> > + IMAFS_DENTRY_IMA_POLICY,
> > + IMAFS_DENTRY_LAST
> > +};
> > +
> > struct ima_namespace {
> > struct kref kref;
> > struct user_namespace *user_ns;
> > @@ -266,6 +277,8 @@ struct ima_namespace {
> > struct mutex ima_write_mutex;
> > unsigned long ima_fs_flags;
> > int valid_policy;
> > +
> > + struct dentry *dentry[IMAFS_DENTRY_LAST];
> > };
> >
> > extern struct ima_namespace init_ima_ns;
> > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > index a749a3e79304..3810d11fb463 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -360,14 +360,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
> > return result;
> > }
> >
> > -static struct dentry *ima_dir;
> > -static struct dentry *ima_symlink;
> > -static struct dentry *binary_runtime_measurements;
> > -static struct dentry *ascii_runtime_measurements;
> > -static struct dentry *runtime_measurements_count;
> > -static struct dentry *violations;
> > -static struct dentry *ima_policy;
> > -
> > enum ima_fs_flags {
> > IMA_FS_BUSY,
> > };
> > @@ -437,8 +429,8 @@ static int ima_release_policy(struct inode *inode, struct file *file)
> >
> > ima_update_policy(ns);
> > #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
> > - securityfs_remove(ima_policy);
> > - ima_policy = NULL;
> > + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
> > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
> > #elif defined(CONFIG_IMA_WRITE_POLICY)
> > clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
> > #elif defined(CONFIG_IMA_READ_POLICY)
> > @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = {
> > .llseek = generic_file_llseek,
> > };
> >
> > -int __init ima_fs_init(void)
> > +static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
> > {
> > - ima_dir = securityfs_create_dir("ima", integrity_dir);
> > - if (IS_ERR(ima_dir))
> > + int i;
> > +
> > + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--)
> > + securityfs_remove(ns->dentry[i]);
> > +
> > + memset(ns->dentry, 0, sizeof(ns->dentry));
> > +}
> > +
> > +static int __init ima_securityfs_init(struct user_namespace *user_ns)
> > +{
> > + struct ima_namespace *ns = user_ns->ima_ns;
> > + struct dentry *ima_dir;
> > +
> > + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir);
> > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR]))
> > return -1;
> > + ima_dir = ns->dentry[IMAFS_DENTRY_DIR];
> >
> > - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
> > - NULL);
> > - if (IS_ERR(ima_symlink))
> > + ns->dentry[IMAFS_DENTRY_SYMLINK] =
> > + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL);
> > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK]))
> > goto out;
> >
> > - binary_runtime_measurements =
> > + ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] =
> > securityfs_create_file("binary_runtime_measurements",
> > S_IRUSR | S_IRGRP, ima_dir, NULL,
> > &ima_measurements_ops);
> > - if (IS_ERR(binary_runtime_measurements))
> > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS]))
> > goto out;
> >
> > - ascii_runtime_measurements =
> > + ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] =
> > securityfs_create_file("ascii_runtime_measurements",
> > S_IRUSR | S_IRGRP, ima_dir, NULL,
> > &ima_ascii_measurements_ops);
> > - if (IS_ERR(ascii_runtime_measurements))
> > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS]))
> > goto out;
> >
> > - runtime_measurements_count =
> > + ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] =
> > securityfs_create_file("runtime_measurements_count",
> > S_IRUSR | S_IRGRP, ima_dir, NULL,
> > &ima_measurements_count_ops);
> > - if (IS_ERR(runtime_measurements_count))
> > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT]))
> > goto out;
> >
> > - violations =
> > + ns->dentry[IMAFS_DENTRY_VIOLATIONS] =
> > securityfs_create_file("violations", S_IRUSR | S_IRGRP,
> > ima_dir, NULL, &ima_htable_violations_ops);
> > - if (IS_ERR(violations))
> > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS]))
> > goto out;
> >
> > - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
> > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] =
> > + securityfs_create_file("policy", POLICY_FILE_FLAGS,
> > ima_dir, NULL,
> > &ima_measure_policy_ops);
> > - if (IS_ERR(ima_policy))
> > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY]))
> > goto out;
> >
> > return 0;
> > out:
> > - securityfs_remove(violations);
> > - securityfs_remove(runtime_measurements_count);
> > - securityfs_remove(ascii_runtime_measurements);
> > - securityfs_remove(binary_runtime_measurements);
> > - securityfs_remove(ima_symlink);
> > - securityfs_remove(ima_dir);
> > - securityfs_remove(ima_policy);
> > + ima_fs_ns_free_dentries(ns);
> > return -1;
> > }
> > +
> > +int __init ima_fs_init(void)
> > +{
> > + return ima_securityfs_init(&init_user_ns);
> > +}
> > --
> > 2.31.1
> >
> >
>

2021-12-09 14:41:28

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Thu, Dec 09, 2021 at 03:37:49PM +0100, Christian Brauner wrote:
> On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
> > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> > > Move the dentries into the ima_namespace for reuse by virtualized
> > > SecurityFS. Implement function freeing the dentries in order of
> > > files and symlinks before directories.
> > >
> > > Signed-off-by: Stefan Berger <[email protected]>
> > > ---
> >
> > This doesn't work as implemented, I think.
> >
> > What I would have preferred and what I tried to explain in the earlier
> > review was:
> > Keep the dentry stashing global since it is only needed for init_ima_ns.
> > Then struct ima_namespace becomes way smaller and simpler.
> > If you do that then it makes sense to remove the additional dget() in
> > securityfs_create_dentry() for non-init_ima_ns.
> > Then you can rely on auto-cleanup in .kill_sb() or on
> > ima_securityfs_init() failure and you only need to call
> > ima_fs_ns_free_dentries() if ns != init_ima_ns.

s/ns != init_ima_ns/ns == init_ima_ns/

> >
> > IIuc, it seems you're currently doing one dput() too many since you're
> > calling securityfs_remove() in the error path for non-init_ima_ns which
> > relies on the previous increased dget() which we removed.
>
> If you really want to move the dentry stashing into struct ima_namespace
> even though it's really unnecessary then you may as well not care about
> the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns)
> call in .kill_sb(). But I really think not dragging dentry stashing into
> struct ima_namespace is the correct way to go about this.
>
> >
> > > include/linux/ima.h | 13 ++++++
> > > security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++---------------
> > > 2 files changed, 52 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > index 3aaf6e806db4..4dd64e318b15 100644
> > > --- a/include/linux/ima.h
> > > +++ b/include/linux/ima.h
> > > @@ -220,6 +220,17 @@ struct ima_h_table {
> > > struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
> > > };
> > >
> > > +enum {
> > > + IMAFS_DENTRY_DIR = 0,
> > > + IMAFS_DENTRY_SYMLINK,
> > > + IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS,
> > > + IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS,
> > > + IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT,
> > > + IMAFS_DENTRY_VIOLATIONS,
> > > + IMAFS_DENTRY_IMA_POLICY,
> > > + IMAFS_DENTRY_LAST
> > > +};
> > > +
> > > struct ima_namespace {
> > > struct kref kref;
> > > struct user_namespace *user_ns;
> > > @@ -266,6 +277,8 @@ struct ima_namespace {
> > > struct mutex ima_write_mutex;
> > > unsigned long ima_fs_flags;
> > > int valid_policy;
> > > +
> > > + struct dentry *dentry[IMAFS_DENTRY_LAST];
> > > };
> > >
> > > extern struct ima_namespace init_ima_ns;
> > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > > index a749a3e79304..3810d11fb463 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -360,14 +360,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
> > > return result;
> > > }
> > >
> > > -static struct dentry *ima_dir;
> > > -static struct dentry *ima_symlink;
> > > -static struct dentry *binary_runtime_measurements;
> > > -static struct dentry *ascii_runtime_measurements;
> > > -static struct dentry *runtime_measurements_count;
> > > -static struct dentry *violations;
> > > -static struct dentry *ima_policy;
> > > -
> > > enum ima_fs_flags {
> > > IMA_FS_BUSY,
> > > };
> > > @@ -437,8 +429,8 @@ static int ima_release_policy(struct inode *inode, struct file *file)
> > >
> > > ima_update_policy(ns);
> > > #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
> > > - securityfs_remove(ima_policy);
> > > - ima_policy = NULL;
> > > + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
> > > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
> > > #elif defined(CONFIG_IMA_WRITE_POLICY)
> > > clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
> > > #elif defined(CONFIG_IMA_READ_POLICY)
> > > @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = {
> > > .llseek = generic_file_llseek,
> > > };
> > >
> > > -int __init ima_fs_init(void)
> > > +static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
> > > {
> > > - ima_dir = securityfs_create_dir("ima", integrity_dir);
> > > - if (IS_ERR(ima_dir))
> > > + int i;
> > > +
> > > + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--)
> > > + securityfs_remove(ns->dentry[i]);
> > > +
> > > + memset(ns->dentry, 0, sizeof(ns->dentry));
> > > +}
> > > +
> > > +static int __init ima_securityfs_init(struct user_namespace *user_ns)
> > > +{
> > > + struct ima_namespace *ns = user_ns->ima_ns;
> > > + struct dentry *ima_dir;
> > > +
> > > + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir);
> > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR]))
> > > return -1;
> > > + ima_dir = ns->dentry[IMAFS_DENTRY_DIR];
> > >
> > > - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
> > > - NULL);
> > > - if (IS_ERR(ima_symlink))
> > > + ns->dentry[IMAFS_DENTRY_SYMLINK] =
> > > + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL);
> > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK]))
> > > goto out;
> > >
> > > - binary_runtime_measurements =
> > > + ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] =
> > > securityfs_create_file("binary_runtime_measurements",
> > > S_IRUSR | S_IRGRP, ima_dir, NULL,
> > > &ima_measurements_ops);
> > > - if (IS_ERR(binary_runtime_measurements))
> > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS]))
> > > goto out;
> > >
> > > - ascii_runtime_measurements =
> > > + ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] =
> > > securityfs_create_file("ascii_runtime_measurements",
> > > S_IRUSR | S_IRGRP, ima_dir, NULL,
> > > &ima_ascii_measurements_ops);
> > > - if (IS_ERR(ascii_runtime_measurements))
> > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS]))
> > > goto out;
> > >
> > > - runtime_measurements_count =
> > > + ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] =
> > > securityfs_create_file("runtime_measurements_count",
> > > S_IRUSR | S_IRGRP, ima_dir, NULL,
> > > &ima_measurements_count_ops);
> > > - if (IS_ERR(runtime_measurements_count))
> > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT]))
> > > goto out;
> > >
> > > - violations =
> > > + ns->dentry[IMAFS_DENTRY_VIOLATIONS] =
> > > securityfs_create_file("violations", S_IRUSR | S_IRGRP,
> > > ima_dir, NULL, &ima_htable_violations_ops);
> > > - if (IS_ERR(violations))
> > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS]))
> > > goto out;
> > >
> > > - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
> > > + ns->dentry[IMAFS_DENTRY_IMA_POLICY] =
> > > + securityfs_create_file("policy", POLICY_FILE_FLAGS,
> > > ima_dir, NULL,
> > > &ima_measure_policy_ops);
> > > - if (IS_ERR(ima_policy))
> > > + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY]))
> > > goto out;
> > >
> > > return 0;
> > > out:
> > > - securityfs_remove(violations);
> > > - securityfs_remove(runtime_measurements_count);
> > > - securityfs_remove(ascii_runtime_measurements);
> > > - securityfs_remove(binary_runtime_measurements);
> > > - securityfs_remove(ima_symlink);
> > > - securityfs_remove(ima_dir);
> > > - securityfs_remove(ima_policy);
> > > + ima_fs_ns_free_dentries(ns);
> > > return -1;
> > > }
> > > +
> > > +int __init ima_fs_init(void)
> > > +{
> > > + return ima_securityfs_init(&init_user_ns);
> > > +}
> > > --
> > > 2.31.1
> > >
> > >
> >
>

2021-12-09 15:01:25

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On 12/9/21 09:41, Christian Brauner wrote:
> On Thu, Dec 09, 2021 at 03:37:49PM +0100, Christian Brauner wrote:
>> On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
>>> On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
>>>> Move the dentries into the ima_namespace for reuse by virtualized
>>>> SecurityFS. Implement function freeing the dentries in order of
>>>> files and symlinks before directories.
>>>>
>>>> Signed-off-by: Stefan Berger <[email protected]>
>>>> ---
>>> This doesn't work as implemented, I think.
>>>
>>> What I would have preferred and what I tried to explain in the earlier
>>> review was:
>>> Keep the dentry stashing global since it is only needed for init_ima_ns.
>>> Then struct ima_namespace becomes way smaller and simpler.
>>> If you do that then it makes sense to remove the additional dget() in
>>> securityfs_create_dentry() for non-init_ima_ns.
>>> Then you can rely on auto-cleanup in .kill_sb() or on
>>> ima_securityfs_init() failure and you only need to call
>>> ima_fs_ns_free_dentries() if ns != init_ima_ns.
> s/ns != init_ima_ns/ns == init_ima_ns/
>
>>> IIuc, it seems you're currently doing one dput() too many since you're
>>> calling securityfs_remove() in the error path for non-init_ima_ns which
>>> relies on the previous increased dget() which we removed.

I thought that securityfs_remove() will now simply influence when a
dentry is removed and freed. If we call it in the error cleanup path in
non-init_user_ns case it would go away right there and leave nothing to
do for .kill_sb() while an additional dget() would require the cleanup
as well but do another cleanup then in .kill_sb() since that brings the
reference count to 0 via the dput()s that it does. Am I wrong on this?

>> If you really want to move the dentry stashing into struct ima_namespace
>> even though it's really unnecessary then you may as well not care about
>> the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns)
>> call in .kill_sb(). But I really think not dragging dentry stashing into
>> struct ima_namespace is the correct way to go about this.

I moved the dentries into the ima_namespace so that each namespace holds
a pointer to the dentries it owns and isolates them. We certainly
wouldn't want to have IMA namespaces write over the current static
variables and create a mess with what these are pointing to (
https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L359
) and possible race conditions when doing parallel initialization (if
that's possible at all). This also reduces the code size and we don't
need two different implementations for init_user_ns and
non-init_user_ns. So I don't quite understand whey we wouldn't want to
have the dentries isolated via ima_namespace?

>>
>>>> include/linux/ima.h | 13 ++++++
>>>> security/integrity/ima/ima_fs.c | 72 ++++++++++++++++++---------------
>>>> 2 files changed, 52 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>>>> index 3aaf6e806db4..4dd64e318b15 100644
>>>> --- a/include/linux/ima.h
>>>> +++ b/include/linux/ima.h
>>>> @@ -220,6 +220,17 @@ struct ima_h_table {
>>>> struct hlist_head queue[IMA_MEASURE_HTABLE_SIZE];
>>>> };
>>>>
>>>> +enum {
>>>> + IMAFS_DENTRY_DIR = 0,
>>>> + IMAFS_DENTRY_SYMLINK,
>>>> + IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS,
>>>> + IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS,
>>>> + IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT,
>>>> + IMAFS_DENTRY_VIOLATIONS,
>>>> + IMAFS_DENTRY_IMA_POLICY,
>>>> + IMAFS_DENTRY_LAST
>>>> +};
>>>> +
>>>> struct ima_namespace {
>>>> struct kref kref;
>>>> struct user_namespace *user_ns;
>>>> @@ -266,6 +277,8 @@ struct ima_namespace {
>>>> struct mutex ima_write_mutex;
>>>> unsigned long ima_fs_flags;
>>>> int valid_policy;
>>>> +
>>>> + struct dentry *dentry[IMAFS_DENTRY_LAST];
>>>> };
>>>>
>>>> extern struct ima_namespace init_ima_ns;
>>>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>>>> index a749a3e79304..3810d11fb463 100644
>>>> --- a/security/integrity/ima/ima_fs.c
>>>> +++ b/security/integrity/ima/ima_fs.c
>>>> @@ -360,14 +360,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>>>> return result;
>>>> }
>>>>
>>>> -static struct dentry *ima_dir;
>>>> -static struct dentry *ima_symlink;
>>>> -static struct dentry *binary_runtime_measurements;
>>>> -static struct dentry *ascii_runtime_measurements;
>>>> -static struct dentry *runtime_measurements_count;
>>>> -static struct dentry *violations;
>>>> -static struct dentry *ima_policy;
>>>> -
>>>> enum ima_fs_flags {
>>>> IMA_FS_BUSY,
>>>> };
>>>> @@ -437,8 +429,8 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>>>>
>>>> ima_update_policy(ns);
>>>> #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
>>>> - securityfs_remove(ima_policy);
>>>> - ima_policy = NULL;
>>>> + securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
>>>> + ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
>>>> #elif defined(CONFIG_IMA_WRITE_POLICY)
>>>> clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
>>>> #elif defined(CONFIG_IMA_READ_POLICY)
>>>> @@ -455,58 +447,72 @@ static const struct file_operations ima_measure_policy_ops = {
>>>> .llseek = generic_file_llseek,
>>>> };
>>>>
>>>> -int __init ima_fs_init(void)
>>>> +static void ima_fs_ns_free_dentries(struct ima_namespace *ns)
>>>> {
>>>> - ima_dir = securityfs_create_dir("ima", integrity_dir);
>>>> - if (IS_ERR(ima_dir))
>>>> + int i;
>>>> +
>>>> + for (i = IMAFS_DENTRY_LAST - 1; i >= 0; i--)
>>>> + securityfs_remove(ns->dentry[i]);
>>>> +
>>>> + memset(ns->dentry, 0, sizeof(ns->dentry));
>>>> +}
>>>> +
>>>> +static int __init ima_securityfs_init(struct user_namespace *user_ns)
>>>> +{
>>>> + struct ima_namespace *ns = user_ns->ima_ns;
>>>> + struct dentry *ima_dir;
>>>> +
>>>> + ns->dentry[IMAFS_DENTRY_DIR] = securityfs_create_dir("ima", integrity_dir);
>>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_DIR]))
>>>> return -1;
>>>> + ima_dir = ns->dentry[IMAFS_DENTRY_DIR];
>>>>
>>>> - ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
>>>> - NULL);
>>>> - if (IS_ERR(ima_symlink))
>>>> + ns->dentry[IMAFS_DENTRY_SYMLINK] =
>>>> + securityfs_create_symlink("ima", NULL, "integrity/ima", NULL);
>>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_SYMLINK]))
>>>> goto out;
>>>>
>>>> - binary_runtime_measurements =
>>>> + ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS] =
>>>> securityfs_create_file("binary_runtime_measurements",
>>>> S_IRUSR | S_IRGRP, ima_dir, NULL,
>>>> &ima_measurements_ops);
>>>> - if (IS_ERR(binary_runtime_measurements))
>>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_BINARY_RUNTIME_MEASUREMENTS]))
>>>> goto out;
>>>>
>>>> - ascii_runtime_measurements =
>>>> + ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS] =
>>>> securityfs_create_file("ascii_runtime_measurements",
>>>> S_IRUSR | S_IRGRP, ima_dir, NULL,
>>>> &ima_ascii_measurements_ops);
>>>> - if (IS_ERR(ascii_runtime_measurements))
>>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_ASCII_RUNTIME_MEASUREMENTS]))
>>>> goto out;
>>>>
>>>> - runtime_measurements_count =
>>>> + ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT] =
>>>> securityfs_create_file("runtime_measurements_count",
>>>> S_IRUSR | S_IRGRP, ima_dir, NULL,
>>>> &ima_measurements_count_ops);
>>>> - if (IS_ERR(runtime_measurements_count))
>>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_RUNTIME_MEASUREMENTS_COUNT]))
>>>> goto out;
>>>>
>>>> - violations =
>>>> + ns->dentry[IMAFS_DENTRY_VIOLATIONS] =
>>>> securityfs_create_file("violations", S_IRUSR | S_IRGRP,
>>>> ima_dir, NULL, &ima_htable_violations_ops);
>>>> - if (IS_ERR(violations))
>>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_VIOLATIONS]))
>>>> goto out;
>>>>
>>>> - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
>>>> + ns->dentry[IMAFS_DENTRY_IMA_POLICY] =
>>>> + securityfs_create_file("policy", POLICY_FILE_FLAGS,
>>>> ima_dir, NULL,
>>>> &ima_measure_policy_ops);
>>>> - if (IS_ERR(ima_policy))
>>>> + if (IS_ERR(ns->dentry[IMAFS_DENTRY_IMA_POLICY]))
>>>> goto out;
>>>>
>>>> return 0;
>>>> out:
>>>> - securityfs_remove(violations);
>>>> - securityfs_remove(runtime_measurements_count);
>>>> - securityfs_remove(ascii_runtime_measurements);
>>>> - securityfs_remove(binary_runtime_measurements);
>>>> - securityfs_remove(ima_symlink);
>>>> - securityfs_remove(ima_dir);
>>>> - securityfs_remove(ima_policy);
>>>> + ima_fs_ns_free_dentries(ns);
>>>> return -1;
>>>> }
>>>> +
>>>> +int __init ima_fs_init(void)
>>>> +{
>>>> + return ima_securityfs_init(&init_user_ns);
>>>> +}
>>>> --
>>>> 2.31.1
>>>>
>>>>

2021-12-09 15:31:02

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote:
> On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
> > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> > > Move the dentries into the ima_namespace for reuse by virtualized
> > > SecurityFS. Implement function freeing the dentries in order of
> > > files and symlinks before directories.
> > >
> > > Signed-off-by: Stefan Berger <[email protected]>
> > > ---
> >
> > This doesn't work as implemented, I think.
> >
> > What I would have preferred and what I tried to explain in the
> > earlier review was:
> > Keep the dentry stashing global since it is only needed for
> > init_ima_ns.
> > Then struct ima_namespace becomes way smaller and simpler.
> > If you do that then it makes sense to remove the additional dget()
> > in securityfs_create_dentry() for non-init_ima_ns.
> > Then you can rely on auto-cleanup in .kill_sb() or on
> > ima_securityfs_init() failure and you only need to call
> > ima_fs_ns_free_dentries() if ns != init_ima_ns.
> >
> > IIuc, it seems you're currently doing one dput() too many since
> > you're calling securityfs_remove() in the error path for non-
> > init_ima_ns which relies on the previous increased dget() which we
> > removed.
>
> If you really want to move the dentry stashing into struct
> ima_namespace even though it's really unnecessary then you may as
> well not care about the auto-cleanup and keep that additional
> ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think
> not dragging dentry stashing into struct ima_namespace is the correct
> way to go about this.

We, unfortunately, do have one case we can't avoid stashing for the
policy file. It's this code in ima_release_policy:

> #if !defined(CONFIG_IMA_WRITE_POLICY) &&
> !defined(CONFIG_IMA_READ_POLICY)
> securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
> ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
>

What it does is that in certain config options, the policy file entry
gets removed from the securityfs ima directory after you write to it.

James

2021-12-09 15:47:18

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Thu, Dec 09, 2021 at 10:00:59AM -0500, Stefan Berger wrote:
>
> On 12/9/21 09:41, Christian Brauner wrote:
> > On Thu, Dec 09, 2021 at 03:37:49PM +0100, Christian Brauner wrote:
> > > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
> > > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> > > > > Move the dentries into the ima_namespace for reuse by virtualized
> > > > > SecurityFS. Implement function freeing the dentries in order of
> > > > > files and symlinks before directories.
> > > > >
> > > > > Signed-off-by: Stefan Berger <[email protected]>
> > > > > ---
> > > > This doesn't work as implemented, I think.
> > > >
> > > > What I would have preferred and what I tried to explain in the earlier
> > > > review was:
> > > > Keep the dentry stashing global since it is only needed for init_ima_ns.
> > > > Then struct ima_namespace becomes way smaller and simpler.
> > > > If you do that then it makes sense to remove the additional dget() in
> > > > securityfs_create_dentry() for non-init_ima_ns.
> > > > Then you can rely on auto-cleanup in .kill_sb() or on
> > > > ima_securityfs_init() failure and you only need to call
> > > > ima_fs_ns_free_dentries() if ns != init_ima_ns.
> > s/ns != init_ima_ns/ns == init_ima_ns/
> >
> > > > IIuc, it seems you're currently doing one dput() too many since you're
> > > > calling securityfs_remove() in the error path for non-init_ima_ns which
> > > > relies on the previous increased dget() which we removed.
>
> I thought that securityfs_remove() will now simply influence when a dentry
> is removed and freed. If we call it in the error cleanup path in
> non-init_user_ns case it would go away right there and leave nothing to do
> for .kill_sb() while an additional dget() would require the cleanup as well
> but do another cleanup then in .kill_sb() since that brings the reference
> count to 0 via the dput()s that it does. Am I wrong on this?

With your change you get one dget() from lookup_one_len() in
securityfs_create_dentry() for non-init_ima_ns. That's added to the
dcache via d_instantiate().
If you call securityfs_dentry_remove() in the error path or anywhere
else it does:

dir = d_inode(dentry->d_parent);
inode_lock(dir);
if (simple_positive(dentry)) {
if (d_is_dir(dentry))
simple_rmdir(dir, dentry);
else
simple_unlink(dir, dentry);
dput(dentry);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That dput() right there is for the additional dget() in
securityfs_create_dentry() but we didn't take that. So the dput() is one
too many now since simple_rmdir() and simple_unlink() will have consumed
one already. (You should be able to easily see this if you compile with
sanitizers on and let your init function fail somewhere in the middle.)

(What usually should happen is sm like this:

void binderfs_remove_file(struct dentry *dentry)
{
struct inode *parent_inode;

parent_inode = d_inode(dentry->d_parent);
inode_lock(parent_inode);
if (simple_positive(dentry)) {
dget(dentry);
simple_unlink(parent_inode, dentry);
d_delete(dentry);
dput(dentry);
}
inode_unlock(parent_inode);
})

>
>
> > > If you really want to move the dentry stashing into struct ima_namespace
> > > even though it's really unnecessary then you may as well not care about
> > > the auto-cleanup and keep that additional ima_fs_ns_free_dentries(ns)
> > > call in .kill_sb(). But I really think not dragging dentry stashing into
> > > struct ima_namespace is the correct way to go about this.
>
>
> I moved the dentries into the ima_namespace so that each namespace holds a
> pointer to the dentries it owns and isolates them. We certainly wouldn't
> want to have IMA namespaces write over the current static variables and
> create a mess with what these are pointing to ( https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L359
> ) and possible race conditions when doing parallel initialization (if that's
> possible at all). This also reduces the code size and we don't need two
> different implementations for init_user_ns and non-init_user_ns. So I don't
> quite understand whey we wouldn't want to have the dentries isolated via
> ima_namespace?

My point was this:
Afaict, nowhere in ima are the stashed dentries needed apart from
ima_policy_release() which is the .release method of the
file_operations where the policy dentry is removed.

The dentries only exist because for pre-namespaced ima if you created a
dentry going through securityfs_create_dentry() you're pinning the
super_block of init-securityfs via simple_pin_fs(). That obliges you to
call securityfs_remove() later on in order to call simple_unpin_fs() for
all dentries.

But for namespaced-ima with namespaced-securityfs there is no more call
to simple_{pin,unpin}_fs(). Consequently you don't need to stash the
dentries anywhere to have them available for removal later on. They will
be automatically cleaned up during .kill_sb().

The one exception I was unaware of reading the code before is in
ima_policy_release(). So apologies, I didn't see that. There you remove:

static int ima_release_policy(struct inode *inode, struct file *file)
{
struct ima_namespace *ns = get_current_ns();
const char *cause = ns->valid_policy ? "completed" : "failed";

if ((file->f_flags & O_ACCMODE) == O_RDONLY)
return seq_release(inode, file);

if (ns->valid_policy && ima_check_policy(ns) < 0) {
cause = "failed";
ns->valid_policy = 0;
}

pr_info("policy update %s\n", cause);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
"policy_update", cause, !ns->valid_policy, 0);

if (!ns->valid_policy) {
ima_delete_rules(ns);
ns->valid_policy = 1;
clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
return 0;
}

ima_update_policy(ns);
#if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;

^^^^^^^^^^^^^^^^^^^^^^^^^

But even so, why then stash all those dentries if the only dentry that
you ever remove while ima is active - and ima isn't a module so can't be
unloaded - is the IMAFS_DENTRY_IMA_POLICY. Simply stash the single
dentry in struct ima_namespace and forget about all the other ones and
avoid wasting memory. But maybe I'm misunderstanding something.

I'm going to get my booster shot and hopefully I'll be able to work
tomorrow and later today but I wouldn't bet on it.

Christian

2021-12-09 19:38:48

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Thu, 2021-12-09 at 10:30 -0500, James Bottomley wrote:
> On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote:
> > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
> > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> > > > Move the dentries into the ima_namespace for reuse by
> > > > virtualized
> > > > SecurityFS. Implement function freeing the dentries in order of
> > > > files and symlinks before directories.
> > > >
> > > > Signed-off-by: Stefan Berger <[email protected]>
> > > > ---
> > >
> > > This doesn't work as implemented, I think.
> > >
> > > What I would have preferred and what I tried to explain in the
> > > earlier review was:
> > > Keep the dentry stashing global since it is only needed for
> > > init_ima_ns.
> > > Then struct ima_namespace becomes way smaller and simpler.
> > > If you do that then it makes sense to remove the additional
> > > dget() in securityfs_create_dentry() for non-init_ima_ns.
> > > Then you can rely on auto-cleanup in .kill_sb() or on
> > > ima_securityfs_init() failure and you only need to call
> > > ima_fs_ns_free_dentries() if ns != init_ima_ns.
> > >
> > > IIuc, it seems you're currently doing one dput() too many since
> > > you're calling securityfs_remove() in the error path for non-
> > > init_ima_ns which relies on the previous increased dget() which
> > > we removed.
> >
> > If you really want to move the dentry stashing into struct
> > ima_namespace even though it's really unnecessary then you may as
> > well not care about the auto-cleanup and keep that additional
> > ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think
> > not dragging dentry stashing into struct ima_namespace is the
> > correct way to go about this.
>
> We, unfortunately, do have one case we can't avoid stashing for the
> policy file. It's this code in ima_release_policy:
>
> > #if !defined(CONFIG_IMA_WRITE_POLICY) &&
> > !defined(CONFIG_IMA_READ_POLICY)
> > securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
> > ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
> >
>
> What it does is that in certain config options, the policy file entry
> gets removed from the securityfs ima directory after you write to it.

This is what I have incremental to v5 that corrects all of this. It
actually keeps every dentry reference (including init_user_ns ones) at
1 so they can be reaped on unmount. For the remove case it does
d_delete and then puts the only reference. This means
securityfs_remove() works for the namespaced policy file as well.

I also got rid of the spurious initialized check in ima_securityfs_init
because it prevents you doing a mount;umount;mount on securityfs within
a namespace.

There's still the problem that if you write the policy, making the file
disappear then unmount and remount securityfs it will come back. My
guess for fixing this is that we only stash the policy file reference,
create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or
something and refuse to create it for that value.

James

---

From 7de285a81ff06b6e0eb2c6db24810aeef9f6dd17 Mon Sep 17 00:00:00 2001
From: James Bottomley <[email protected]>
Date: Thu, 9 Dec 2021 19:33:49 +0000
Subject: [PATCH] fix dentry ref counting

---
security/inode.c | 12 ++----------
security/integrity/ima/ima_fs.c | 4 ----
2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/security/inode.c b/security/inode.c
index eaccba7017d9..b53152f7a625 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -178,8 +178,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
inode->i_fop = fops;
}
d_instantiate(dentry, inode);
- if (ns == &init_user_ns)
- dget(dentry);
inode_unlock(dir);
return dentry;

@@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink);
void securityfs_remove(struct dentry *dentry)
{
struct user_namespace *ns = dentry->d_sb->s_user_ns;
- struct inode *dir;

if (!dentry || IS_ERR(dentry))
return;

- dir = d_inode(dentry->d_parent);
- inode_lock(dir);
if (simple_positive(dentry)) {
- if (d_is_dir(dentry))
- simple_rmdir(dir, dentry);
- else
- simple_unlink(dir, dentry);
+ d_delete(dentry);
dput(dentry);
}
- inode_unlock(dir);
+
if (ns == &init_user_ns)
simple_release_fs(&init_securityfs_mount,
&init_securityfs_mount_count);
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 778983fd9a73..077a6ff46858 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -466,10 +466,6 @@ int ima_securityfs_init(struct user_namespace *user_ns, struct dentry *root)
struct ima_namespace *ns = user_ns->ima_ns;
struct dentry *ima_dir;

- /* already initialized? */
- if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])
- return 0;
-
/* FIXME: update when evm and integrity are namespaced */
if (user_ns != &init_user_ns) {
ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =
--
2.33.0

2021-12-09 20:13:31

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On 12/9/21 14:38, James Bottomley wrote:
> On Thu, 2021-12-09 at 10:30 -0500, James Bottomley wrote:
>> On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote:
>>> On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
>>>> On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
>>>>> Move the dentries into the ima_namespace for reuse by
>>>>> virtualized
>>>>> SecurityFS. Implement function freeing the dentries in order of
>>>>> files and symlinks before directories.
>>>>>
>>>>> Signed-off-by: Stefan Berger <[email protected]>
>>>>> ---
>>>> This doesn't work as implemented, I think.
>>>>
>>>> What I would have preferred and what I tried to explain in the
>>>> earlier review was:
>>>> Keep the dentry stashing global since it is only needed for
>>>> init_ima_ns.
>>>> Then struct ima_namespace becomes way smaller and simpler.
>>>> If you do that then it makes sense to remove the additional
>>>> dget() in securityfs_create_dentry() for non-init_ima_ns.
>>>> Then you can rely on auto-cleanup in .kill_sb() or on
>>>> ima_securityfs_init() failure and you only need to call
>>>> ima_fs_ns_free_dentries() if ns != init_ima_ns.
>>>>
>>>> IIuc, it seems you're currently doing one dput() too many since
>>>> you're calling securityfs_remove() in the error path for non-
>>>> init_ima_ns which relies on the previous increased dget() which
>>>> we removed.
>>> If you really want to move the dentry stashing into struct
>>> ima_namespace even though it's really unnecessary then you may as
>>> well not care about the auto-cleanup and keep that additional
>>> ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think
>>> not dragging dentry stashing into struct ima_namespace is the
>>> correct way to go about this.
>> We, unfortunately, do have one case we can't avoid stashing for the
>> policy file. It's this code in ima_release_policy:
>>
>>> #if !defined(CONFIG_IMA_WRITE_POLICY) &&
>>> !defined(CONFIG_IMA_READ_POLICY)
>>> securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
>>> ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
>>>
>> What it does is that in certain config options, the policy file entry
>> gets removed from the securityfs ima directory after you write to it.
> This is what I have incremental to v5 that corrects all of this. It
> actually keeps every dentry reference (including init_user_ns ones) at
> 1 so they can be reaped on unmount. For the remove case it does
> d_delete and then puts the only reference. This means
> securityfs_remove() works for the namespaced policy file as well.
I fixed it now as well but do another dget() on securityfs_removed() for
ns != init_user_ns. Ok, I will add what you have below.
>
> I also got rid of the spurious initialized check in ima_securityfs_init
> because it prevents you doing a mount;umount;mount on securityfs within
> a namespace.
>
> There's still the problem that if you write the policy, making the file
> disappear then unmount and remount securityfs it will come back. My
> guess for fixing this is that we only stash the policy file reference,
> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or
> something and refuse to create it for that value.

What about boolean to remember this? I just added this.

>
> James
>
> ---
>
> From 7de285a81ff06b6e0eb2c6db24810aeef9f6dd17 Mon Sep 17 00:00:00 2001
> From: James Bottomley <[email protected]>
> Date: Thu, 9 Dec 2021 19:33:49 +0000
> Subject: [PATCH] fix dentry ref counting
>
> ---
> security/inode.c | 12 ++----------
> security/integrity/ima/ima_fs.c | 4 ----
> 2 files changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/security/inode.c b/security/inode.c
> index eaccba7017d9..b53152f7a625 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -178,8 +178,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
> inode->i_fop = fops;
> }
> d_instantiate(dentry, inode);
> - if (ns == &init_user_ns)
> - dget(dentry);
> inode_unlock(dir);
> return dentry;
>
> @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink);
> void securityfs_remove(struct dentry *dentry)
> {
> struct user_namespace *ns = dentry->d_sb->s_user_ns;
> - struct inode *dir;
>
> if (!dentry || IS_ERR(dentry))
> return;
>
> - dir = d_inode(dentry->d_parent);
> - inode_lock(dir);
> if (simple_positive(dentry)) {
> - if (d_is_dir(dentry))
> - simple_rmdir(dir, dentry);
> - else
> - simple_unlink(dir, dentry);
> + d_delete(dentry);
> dput(dentry);
> }
> - inode_unlock(dir);
> +
> if (ns == &init_user_ns)
> simple_release_fs(&init_securityfs_mount,
> &init_securityfs_mount_count);
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 778983fd9a73..077a6ff46858 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -466,10 +466,6 @@ int ima_securityfs_init(struct user_namespace *user_ns, struct dentry *root)
> struct ima_namespace *ns = user_ns->ima_ns;
> struct dentry *ima_dir;
>
> - /* already initialized? */
> - if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])
> - return 0;
> -
> /* FIXME: update when evm and integrity are namespaced */
> if (user_ns != &init_user_ns) {
> ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =

2021-12-10 11:49:49

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Thu, Dec 09, 2021 at 02:38:13PM -0500, James Bottomley wrote:
> On Thu, 2021-12-09 at 10:30 -0500, James Bottomley wrote:
> > On Thu, 2021-12-09 at 15:37 +0100, Christian Brauner wrote:
> > > On Thu, Dec 09, 2021 at 03:34:28PM +0100, Christian Brauner wrote:
> > > > On Wed, Dec 08, 2021 at 05:18:17PM -0500, Stefan Berger wrote:
> > > > > Move the dentries into the ima_namespace for reuse by
> > > > > virtualized
> > > > > SecurityFS. Implement function freeing the dentries in order of
> > > > > files and symlinks before directories.
> > > > >
> > > > > Signed-off-by: Stefan Berger <[email protected]>
> > > > > ---
> > > >
> > > > This doesn't work as implemented, I think.
> > > >
> > > > What I would have preferred and what I tried to explain in the
> > > > earlier review was:
> > > > Keep the dentry stashing global since it is only needed for
> > > > init_ima_ns.
> > > > Then struct ima_namespace becomes way smaller and simpler.
> > > > If you do that then it makes sense to remove the additional
> > > > dget() in securityfs_create_dentry() for non-init_ima_ns.
> > > > Then you can rely on auto-cleanup in .kill_sb() or on
> > > > ima_securityfs_init() failure and you only need to call
> > > > ima_fs_ns_free_dentries() if ns != init_ima_ns.
> > > >
> > > > IIuc, it seems you're currently doing one dput() too many since
> > > > you're calling securityfs_remove() in the error path for non-
> > > > init_ima_ns which relies on the previous increased dget() which
> > > > we removed.
> > >
> > > If you really want to move the dentry stashing into struct
> > > ima_namespace even though it's really unnecessary then you may as
> > > well not care about the auto-cleanup and keep that additional
> > > ima_fs_ns_free_dentries(ns) call in .kill_sb(). But I really think
> > > not dragging dentry stashing into struct ima_namespace is the
> > > correct way to go about this.
> >
> > We, unfortunately, do have one case we can't avoid stashing for the
> > policy file. It's this code in ima_release_policy:
> >
> > > #if !defined(CONFIG_IMA_WRITE_POLICY) &&
> > > !defined(CONFIG_IMA_READ_POLICY)
> > > securityfs_remove(ns->dentry[IMAFS_DENTRY_IMA_POLICY]);
> > > ns->dentry[IMAFS_DENTRY_IMA_POLICY] = NULL;
> > >
> >
> > What it does is that in certain config options, the policy file entry
> > gets removed from the securityfs ima directory after you write to it.
>
> This is what I have incremental to v5 that corrects all of this. It
> actually keeps every dentry reference (including init_user_ns ones) at
> 1 so they can be reaped on unmount. For the remove case it does
> d_delete and then puts the only reference. This means
> securityfs_remove() works for the namespaced policy file as well.
>
> I also got rid of the spurious initialized check in ima_securityfs_init
> because it prevents you doing a mount;umount;mount on securityfs within
> a namespace.
>
> There's still the problem that if you write the policy, making the file
> disappear then unmount and remount securityfs it will come back. My
> guess for fixing this is that we only stash the policy file reference,
> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or
> something and refuse to create it for that value.

Some sort of indicator that gets stashed in struct ima_ns that the file
does not get recreated on consecutive mounts. That shouldn't be hard to
fix.

>
> James
>
> ---
>
> From 7de285a81ff06b6e0eb2c6db24810aeef9f6dd17 Mon Sep 17 00:00:00 2001
> From: James Bottomley <[email protected]>
> Date: Thu, 9 Dec 2021 19:33:49 +0000
> Subject: [PATCH] fix dentry ref counting
>
> ---
> security/inode.c | 12 ++----------
> security/integrity/ima/ima_fs.c | 4 ----
> 2 files changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/security/inode.c b/security/inode.c
> index eaccba7017d9..b53152f7a625 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -178,8 +178,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
> inode->i_fop = fops;
> }
> d_instantiate(dentry, inode);
> - if (ns == &init_user_ns)
> - dget(dentry);
> inode_unlock(dir);
> return dentry;
>
> @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink);
> void securityfs_remove(struct dentry *dentry)
> {
> struct user_namespace *ns = dentry->d_sb->s_user_ns;
> - struct inode *dir;
>
> if (!dentry || IS_ERR(dentry))
> return;
>
> - dir = d_inode(dentry->d_parent);
> - inode_lock(dir);
> if (simple_positive(dentry)) {
> - if (d_is_dir(dentry))
> - simple_rmdir(dir, dentry);
> - else
> - simple_unlink(dir, dentry);
> + d_delete(dentry);

Not, that doesn't work. You can't just call d_delete() and dput() and
even if I wouldn't advise it. And you also can't do this without taking
the inode lock on the directory.
simple_rmdir()/simple_unlink() take care to update various inode fields
in the parent dir and handle link counts. This really wants to be sm
like

struct inode *parent_inode;

parent_inode = d_inode(dentry->d_parent);
inode_lock(parent_inode);
if (simple_positive(dentry)) {
dget(dentry);
if (d_is_dir(dentry)
simple_unlink(parent_inode, dentry);
else
simple_unlink(parent_inode, dentry);
d_delete(dentry);
dput(dentry);
}
inode_unlock(parent_inode);

> dput(dentry);
> }
> - inode_unlock(dir);
> +
> if (ns == &init_user_ns)
> simple_release_fs(&init_securityfs_mount,
> &init_securityfs_mount_count);
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 778983fd9a73..077a6ff46858 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -466,10 +466,6 @@ int ima_securityfs_init(struct user_namespace *user_ns, struct dentry *root)
> struct ima_namespace *ns = user_ns->ima_ns;
> struct dentry *ima_dir;
>
> - /* already initialized? */
> - if (ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR])
> - return 0;
> -
> /* FIXME: update when evm and integrity are namespaced */
> if (user_ns != &init_user_ns) {
> ns->dentry[IMAFS_DENTRY_INTEGRITY_DIR] =
> --
> 2.33.0
>
>
>

2021-12-10 12:10:16

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> > There's still the problem that if you write the policy, making the file
> > disappear then unmount and remount securityfs it will come back. My
> > guess for fixing this is that we only stash the policy file reference,
> > create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or
> > something and refuse to create it for that value.
>
> Some sort of indicator that gets stashed in struct ima_ns that the file
> does not get recreated on consecutive mounts. That shouldn't be hard to
> fix.

The policy file disappearing is for backwards compatibility, prior to
being able to extend the custom policy. For embedded usecases,
allowing the policy to be written exactly once might makes sense. Do
we really want/need to continue to support removing the policy in
namespaces?

thanks,

Mimi

2021-12-10 12:40:26

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On 12/10/21 07:09, Mimi Zohar wrote:
> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
>>> There's still the problem that if you write the policy, making the file
>>> disappear then unmount and remount securityfs it will come back. My
>>> guess for fixing this is that we only stash the policy file reference,
>>> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or
>>> something and refuse to create it for that value.
>> Some sort of indicator that gets stashed in struct ima_ns that the file
>> does not get recreated on consecutive mounts. That shouldn't be hard to
>> fix.
> The policy file disappearing is for backwards compatibility, prior to
> being able to extend the custom policy. For embedded usecases,
> allowing the policy to be written exactly once might makes sense. Do
> we really want/need to continue to support removing the policy in
> namespaces?

I don't have an answer but should the behavior for the same #define in
this case be different for host and namespaces? Or should we just
'select IMA_WRITE_POLICY and IMA_READ_POLICY' when IMA_NS is selected?

>
> thanks,
>
> Mimi
>

2021-12-10 12:41:03

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 07:09 -0500, Mimi Zohar wrote:
> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> > > There's still the problem that if you write the policy, making
> > > the file disappear then unmount and remount securityfs it will
> > > come back. My guess for fixing this is that we only stash the
> > > policy file reference, create it if NULL but then set the pointer
> > > to PTR_ERR(-EINVAL) or something and refuse to create it for that
> > > value.
> >
> > Some sort of indicator that gets stashed in struct ima_ns that the
> > file does not get recreated on consecutive mounts. That shouldn't
> > be hard to fix.

Yes, Stefan said he was doing that.

> The policy file disappearing is for backwards compatibility, prior to
> being able to extend the custom policy. For embedded usecases,
> allowing the policy to be written exactly once might makes sense. Do
> we really want/need to continue to support removing the policy in
> namespaces?

The embedded world tends also to be a big consumer of namespaces, so if
this semantic is for them, likely it should remain in the namespaced
IMA.

But how necessary is the semantic? If we got rid of it from the whole
of IMA, what would break? If we can't think of anything it could likely
be removed from both namespaced and non-namespaced IMA.

James

2021-12-10 12:55:09

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 07:40 -0500, James Bottomley wrote:
> On Fri, 2021-12-10 at 07:09 -0500, Mimi Zohar wrote:
> > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> > > > There's still the problem that if you write the policy, making
> > > > the file disappear then unmount and remount securityfs it will
> > > > come back. My guess for fixing this is that we only stash the
> > > > policy file reference, create it if NULL but then set the pointer
> > > > to PTR_ERR(-EINVAL) or something and refuse to create it for that
> > > > value.
> > >
> > > Some sort of indicator that gets stashed in struct ima_ns that the
> > > file does not get recreated on consecutive mounts. That shouldn't
> > > be hard to fix.
>
> Yes, Stefan said he was doing that.
>
> > The policy file disappearing is for backwards compatibility, prior to
> > being able to extend the custom policy. For embedded usecases,
> > allowing the policy to be written exactly once might makes sense. Do
> > we really want/need to continue to support removing the policy in
> > namespaces?
>
> The embedded world tends also to be a big consumer of namespaces, so if
> this semantic is for them, likely it should remain in the namespaced
> IMA.

Think of a simple device that loads a custom IMA policy, which never
changes once loaded.
>
> But how necessary is the semantic? If we got rid of it from the whole
> of IMA, what would break? If we can't think of anything it could likely
> be removed from both namespaced and non-namespaced IMA.

The question isn't an issue of "breaking", but of leaking info. If
this isn't a real concern, then the ability of removing the securityfs
isn't needed.

thanks,

Mimi

2021-12-10 13:02:44

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote:
> On 12/10/21 07:09, Mimi Zohar wrote:
> > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> >>> There's still the problem that if you write the policy, making the file
> >>> disappear then unmount and remount securityfs it will come back. My
> >>> guess for fixing this is that we only stash the policy file reference,
> >>> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or
> >>> something and refuse to create it for that value.
> >> Some sort of indicator that gets stashed in struct ima_ns that the file
> >> does not get recreated on consecutive mounts. That shouldn't be hard to
> >> fix.
> > The policy file disappearing is for backwards compatibility, prior to
> > being able to extend the custom policy. For embedded usecases,
> > allowing the policy to be written exactly once might makes sense. Do
> > we really want/need to continue to support removing the policy in
> > namespaces?
>
> I don't have an answer but should the behavior for the same #define in
> this case be different for host and namespaces? Or should we just
> 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when IMA_NS is selected?

The latter option sounds good. Being able to analyze the namespace
policy is really important.

thanks,

Mimi

2021-12-10 14:17:39

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On 12/10/21 08:02, Mimi Zohar wrote:
> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote:
>> On 12/10/21 07:09, Mimi Zohar wrote:
>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
>>>>> There's still the problem that if you write the policy, making the file
>>>>> disappear then unmount and remount securityfs it will come back. My
>>>>> guess for fixing this is that we only stash the policy file reference,
>>>>> create it if NULL but then set the pointer to PTR_ERR(-EINVAL) or
>>>>> something and refuse to create it for that value.
>>>> Some sort of indicator that gets stashed in struct ima_ns that the file
>>>> does not get recreated on consecutive mounts. That shouldn't be hard to
>>>> fix.
>>> The policy file disappearing is for backwards compatibility, prior to
>>> being able to extend the custom policy. For embedded usecases,
>>> allowing the policy to be written exactly once might makes sense. Do
>>> we really want/need to continue to support removing the policy in
>>> namespaces?
>> I don't have an answer but should the behavior for the same #define in
>> this case be different for host and namespaces? Or should we just
>> 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when IMA_NS is selected?
> The latter option sounds good. Being able to analyze the namespace
> policy is really important.

Ok, I will adjust the Kconfig for this then. This then warrants the
question whether to move the dentry into the ima_namespace. The current
code looks like this.

#if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
        securityfs_remove(ns->policy_dentry);
        ns->policy_dentry = NULL;
        ns->policy_dentry_removed = true;
#elif defined(CONFIG_IMA_WRITE_POLICY)

With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above
wouldn't be necessary anymore but I find it 'cleaner' to still have the
dentry isolated rather than it being a global static as it was before...

>
> thanks,
>
> Mimi
>

2021-12-10 14:26:49

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote:
> On 12/10/21 08:02, Mimi Zohar wrote:
> > On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote:
> > > On 12/10/21 07:09, Mimi Zohar wrote:
> > > > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> > > > > > There's still the problem that if you write the policy,
> > > > > > making the file disappear then unmount and remount
> > > > > > securityfs it will come back. My guess for fixing this is
> > > > > > that we only stash the policy file reference,
> > > > > > create it if NULL but then set the pointer to PTR_ERR(-
> > > > > > EINVAL) or something and refuse to create it for that
> > > > > > value.
> > > > > Some sort of indicator that gets stashed in struct ima_ns
> > > > > that the file does not get recreated on consecutive mounts.
> > > > > That shouldn't be hard to fix.
> > > > The policy file disappearing is for backwards compatibility,
> > > > prior to being able to extend the custom policy. For embedded
> > > > usecases, allowing the policy to be written exactly once might
> > > > makes sense. Do we really want/need to continue to support
> > > > removing the policy in namespaces?
> > > I don't have an answer but should the behavior for the same
> > > #define in this case be different for host and namespaces? Or
> > > should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when
> > > IMA_NS is selected?
> > The latter option sounds good. Being able to analyze the namespace
> > policy is really important.
>
> Ok, I will adjust the Kconfig for this then. This then warrants the
> question whether to move the dentry into the ima_namespace. The
> current code looks like this.
>
> #if !defined(CONFIG_IMA_WRITE_POLICY) &&
> !defined(CONFIG_IMA_READ_POLICY)
> securityfs_remove(ns->policy_dentry);
> ns->policy_dentry = NULL;
> ns->policy_dentry_removed = true;
> #elif defined(CONFIG_IMA_WRITE_POLICY)
>
> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above
> wouldn't be necessary anymore but I find it 'cleaner' to still have
> the dentry isolated rather than it being a global static as it was
> before...

This is really, really why you don't want the semantics inside the
namespace to differ from those outside, because it creates confusion
for the people reading the code, especially with magically forced
config options like this. I'd strongly suggest you either keep the
semantic in the namespace or eliminate it entirely.

If you really, really have to make the namespace behave differently,
then use global variables and put a big comment on that code saying it
can never be reached once CONFIG_IMA_NS is enabled.

James

2021-12-10 15:27:20

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote:
> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote:
> > On 12/10/21 08:02, Mimi Zohar wrote:
> > > On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote:
> > > > On 12/10/21 07:09, Mimi Zohar wrote:
> > > > > On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> > > > > > > There's still the problem that if you write the policy,
> > > > > > > making the file disappear then unmount and remount
> > > > > > > securityfs it will come back. My guess for fixing this is
> > > > > > > that we only stash the policy file reference,
> > > > > > > create it if NULL but then set the pointer to PTR_ERR(-
> > > > > > > EINVAL) or something and refuse to create it for that
> > > > > > > value.
> > > > > > Some sort of indicator that gets stashed in struct ima_ns
> > > > > > that the file does not get recreated on consecutive mounts.
> > > > > > That shouldn't be hard to fix.
> > > > > The policy file disappearing is for backwards compatibility,
> > > > > prior to being able to extend the custom policy. For embedded
> > > > > usecases, allowing the policy to be written exactly once might
> > > > > makes sense. Do we really want/need to continue to support
> > > > > removing the policy in namespaces?
> > > > I don't have an answer but should the behavior for the same
> > > > #define in this case be different for host and namespaces? Or
> > > > should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when
> > > > IMA_NS is selected?
> > > The latter option sounds good. Being able to analyze the namespace
> > > policy is really important.
> >
> > Ok, I will adjust the Kconfig for this then. This then warrants the
> > question whether to move the dentry into the ima_namespace. The
> > current code looks like this.
> >
> > #if !defined(CONFIG_IMA_WRITE_POLICY) &&
> > !defined(CONFIG_IMA_READ_POLICY)
> > securityfs_remove(ns->policy_dentry);
> > ns->policy_dentry = NULL;
> > ns->policy_dentry_removed = true;
> > #elif defined(CONFIG_IMA_WRITE_POLICY)
> >
> > With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above
> > wouldn't be necessary anymore but I find it 'cleaner' to still have
> > the dentry isolated rather than it being a global static as it was
> > before...
>
> This is really, really why you don't want the semantics inside the
> namespace to differ from those outside, because it creates confusion
> for the people reading the code, especially with magically forced
> config options like this. I'd strongly suggest you either keep the
> semantic in the namespace or eliminate it entirely.
>
> If you really, really have to make the namespace behave differently,
> then use global variables and put a big comment on that code saying it
> can never be reached once CONFIG_IMA_NS is enabled.

The problem seems to be with removing the securityfs policy file.
Instead of removing it, just make it inacessible for the "if
!defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)"
case.

thanks,

Mimi

2021-12-10 15:32:57

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On 12/10/21 10:26, Mimi Zohar wrote:
> On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote:
>> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote:
>>> On 12/10/21 08:02, Mimi Zohar wrote:
>>>> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote:
>>>>> On 12/10/21 07:09, Mimi Zohar wrote:
>>>>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
>>>>>>>> There's still the problem that if you write the policy,
>>>>>>>> making the file disappear then unmount and remount
>>>>>>>> securityfs it will come back. My guess for fixing this is
>>>>>>>> that we only stash the policy file reference,
>>>>>>>> create it if NULL but then set the pointer to PTR_ERR(-
>>>>>>>> EINVAL) or something and refuse to create it for that
>>>>>>>> value.
>>>>>>> Some sort of indicator that gets stashed in struct ima_ns
>>>>>>> that the file does not get recreated on consecutive mounts.
>>>>>>> That shouldn't be hard to fix.
>>>>>> The policy file disappearing is for backwards compatibility,
>>>>>> prior to being able to extend the custom policy. For embedded
>>>>>> usecases, allowing the policy to be written exactly once might
>>>>>> makes sense. Do we really want/need to continue to support
>>>>>> removing the policy in namespaces?
>>>>> I don't have an answer but should the behavior for the same
>>>>> #define in this case be different for host and namespaces? Or
>>>>> should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when
>>>>> IMA_NS is selected?
>>>> The latter option sounds good. Being able to analyze the namespace
>>>> policy is really important.
>>> Ok, I will adjust the Kconfig for this then. This then warrants the
>>> question whether to move the dentry into the ima_namespace. The
>>> current code looks like this.
>>>
>>> #if !defined(CONFIG_IMA_WRITE_POLICY) &&
>>> !defined(CONFIG_IMA_READ_POLICY)
>>> securityfs_remove(ns->policy_dentry);
>>> ns->policy_dentry = NULL;
>>> ns->policy_dentry_removed = true;
>>> #elif defined(CONFIG_IMA_WRITE_POLICY)
>>>
>>> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above
>>> wouldn't be necessary anymore but I find it 'cleaner' to still have
>>> the dentry isolated rather than it being a global static as it was
>>> before...
>> This is really, really why you don't want the semantics inside the
>> namespace to differ from those outside, because it creates confusion
>> for the people reading the code, especially with magically forced
>> config options like this. I'd strongly suggest you either keep the
>> semantic in the namespace or eliminate it entirely.
>>
>> If you really, really have to make the namespace behave differently,
>> then use global variables and put a big comment on that code saying it
>> can never be reached once CONFIG_IMA_NS is enabled.
> The problem seems to be with removing the securityfs policy file.
> Instead of removing it, just make it inacessible for the "if
> !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)"
> case.

So we would then leave it up to the one building the kernel to select
the proper compile time options (suggested ones being IMA_WRITE_POLICY
and IMA_READ_POLICY being enabled?) and behavior of host and IMA
namespace is then the same per those options? Removing the file didn't
seem the problem to me but more like whether the host should ever behave
differently from the namespace.

Stefan

>
> thanks,
>
> Mimi
>

2021-12-10 15:49:41

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 10:32 -0500, Stefan Berger wrote:
> On 12/10/21 10:26, Mimi Zohar wrote:
> > On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote:
> >> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote:
> >>> On 12/10/21 08:02, Mimi Zohar wrote:
> >>>> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote:
> >>>>> On 12/10/21 07:09, Mimi Zohar wrote:
> >>>>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> >>>>>>>> There's still the problem that if you write the policy,
> >>>>>>>> making the file disappear then unmount and remount
> >>>>>>>> securityfs it will come back. My guess for fixing this is
> >>>>>>>> that we only stash the policy file reference,
> >>>>>>>> create it if NULL but then set the pointer to PTR_ERR(-
> >>>>>>>> EINVAL) or something and refuse to create it for that
> >>>>>>>> value.
> >>>>>>> Some sort of indicator that gets stashed in struct ima_ns
> >>>>>>> that the file does not get recreated on consecutive mounts.
> >>>>>>> That shouldn't be hard to fix.
> >>>>>> The policy file disappearing is for backwards compatibility,
> >>>>>> prior to being able to extend the custom policy. For embedded
> >>>>>> usecases, allowing the policy to be written exactly once might
> >>>>>> makes sense. Do we really want/need to continue to support
> >>>>>> removing the policy in namespaces?
> >>>>> I don't have an answer but should the behavior for the same
> >>>>> #define in this case be different for host and namespaces? Or
> >>>>> should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when
> >>>>> IMA_NS is selected?
> >>>> The latter option sounds good. Being able to analyze the namespace
> >>>> policy is really important.
> >>> Ok, I will adjust the Kconfig for this then. This then warrants the
> >>> question whether to move the dentry into the ima_namespace. The
> >>> current code looks like this.
> >>>
> >>> #if !defined(CONFIG_IMA_WRITE_POLICY) &&
> >>> !defined(CONFIG_IMA_READ_POLICY)
> >>> securityfs_remove(ns->policy_dentry);
> >>> ns->policy_dentry = NULL;
> >>> ns->policy_dentry_removed = true;
> >>> #elif defined(CONFIG_IMA_WRITE_POLICY)
> >>>
> >>> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above
> >>> wouldn't be necessary anymore but I find it 'cleaner' to still have
> >>> the dentry isolated rather than it being a global static as it was
> >>> before...
> >> This is really, really why you don't want the semantics inside the
> >> namespace to differ from those outside, because it creates confusion
> >> for the people reading the code, especially with magically forced
> >> config options like this. I'd strongly suggest you either keep the
> >> semantic in the namespace or eliminate it entirely.
> >>
> >> If you really, really have to make the namespace behave differently,
> >> then use global variables and put a big comment on that code saying it
> >> can never be reached once CONFIG_IMA_NS is enabled.
> > The problem seems to be with removing the securityfs policy file.
> > Instead of removing it, just make it inacessible for the "if
> > !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)"
> > case.
>
> So we would then leave it up to the one building the kernel to select
> the proper compile time options (suggested ones being IMA_WRITE_POLICY
> and IMA_READ_POLICY being enabled?) and behavior of host and IMA
> namespace is then the same per those options? Removing the file didn't
> seem the problem to me but more like whether the host should ever behave
> differently from the namespace.

You proposed "select IMA_WRITE_POLICY and IMA_READ_POLICY'" when IMA_NS
is selected. At least IMA_READ_POLICY should be enabled for
namespaces.

In addition, if removing the securityfs file after a custom policy is
loaded complicates namespacing, then don't remove it.

thanks,

Mimi

2021-12-10 16:40:41

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On 12/10/21 10:48, Mimi Zohar wrote:
> On Fri, 2021-12-10 at 10:32 -0500, Stefan Berger wrote:
>> On 12/10/21 10:26, Mimi Zohar wrote:
>>> On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote:
>>>> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote:
>>>>> On 12/10/21 08:02, Mimi Zohar wrote:
>>>>>> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote:
>>>>>>> On 12/10/21 07:09, Mimi Zohar wrote:
>>>>>>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
>>>>>>>>>> There's still the problem that if you write the policy,
>>>>>>>>>> making the file disappear then unmount and remount
>>>>>>>>>> securityfs it will come back. My guess for fixing this is
>>>>>>>>>> that we only stash the policy file reference,
>>>>>>>>>> create it if NULL but then set the pointer to PTR_ERR(-
>>>>>>>>>> EINVAL) or something and refuse to create it for that
>>>>>>>>>> value.
>>>>>>>>> Some sort of indicator that gets stashed in struct ima_ns
>>>>>>>>> that the file does not get recreated on consecutive mounts.
>>>>>>>>> That shouldn't be hard to fix.
>>>>>>>> The policy file disappearing is for backwards compatibility,
>>>>>>>> prior to being able to extend the custom policy. For embedded
>>>>>>>> usecases, allowing the policy to be written exactly once might
>>>>>>>> makes sense. Do we really want/need to continue to support
>>>>>>>> removing the policy in namespaces?
>>>>>>> I don't have an answer but should the behavior for the same
>>>>>>> #define in this case be different for host and namespaces? Or
>>>>>>> should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when
>>>>>>> IMA_NS is selected?
>>>>>> The latter option sounds good. Being able to analyze the namespace
>>>>>> policy is really important.
>>>>> Ok, I will adjust the Kconfig for this then. This then warrants the
>>>>> question whether to move the dentry into the ima_namespace. The
>>>>> current code looks like this.
>>>>>
>>>>> #if !defined(CONFIG_IMA_WRITE_POLICY) &&
>>>>> !defined(CONFIG_IMA_READ_POLICY)
>>>>> securityfs_remove(ns->policy_dentry);
>>>>> ns->policy_dentry = NULL;
>>>>> ns->policy_dentry_removed = true;
>>>>> #elif defined(CONFIG_IMA_WRITE_POLICY)
>>>>>
>>>>> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above
>>>>> wouldn't be necessary anymore but I find it 'cleaner' to still have
>>>>> the dentry isolated rather than it being a global static as it was
>>>>> before...
>>>> This is really, really why you don't want the semantics inside the
>>>> namespace to differ from those outside, because it creates confusion
>>>> for the people reading the code, especially with magically forced
>>>> config options like this. I'd strongly suggest you either keep the
>>>> semantic in the namespace or eliminate it entirely.
>>>>
>>>> If you really, really have to make the namespace behave differently,
>>>> then use global variables and put a big comment on that code saying it
>>>> can never be reached once CONFIG_IMA_NS is enabled.
>>> The problem seems to be with removing the securityfs policy file.
>>> Instead of removing it, just make it inacessible for the "if
>>> !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)"
>>> case.
>> So we would then leave it up to the one building the kernel to select
>> the proper compile time options (suggested ones being IMA_WRITE_POLICY
>> and IMA_READ_POLICY being enabled?) and behavior of host and IMA
>> namespace is then the same per those options? Removing the file didn't
>> seem the problem to me but more like whether the host should ever behave
>> differently from the namespace.
> You proposed "select IMA_WRITE_POLICY and IMA_READ_POLICY'" when IMA_NS
> is selected. At least IMA_READ_POLICY should be enabled for
> namespaces.
>
> In addition, if removing the securityfs file after a custom policy is
> loaded complicates namespacing, then don't remove it.

If we just leave the selection of the compile time options to the user
(suggested ones being IMA_WRITE_POLICY

and IMA_READ_POLICY being enabled) we don't need to make any changes.

Choices are for IMA_NS:
1) Leave compile-time options to the user and suggest IMA_WRITE_POLICY and IMA_READ_POLICY
2) Auto-select IMA_WRITE_POLICY and IMA_READ_POLICY for IMA_NS and still remove file for non-IMA_NS case and have the dentry in the ima_namespace
3) Auto-select IMA_WRITE_POLICY and IMA_READ_POLICY for IMA_NS and still remove file for non-IMA_NS case and use global dentry
4) Changing mode bits on the file/inode to avoid having the dentry

for v6 I just leave things as they are and we can then see what we need.

>
> thanks,
>
> Mimi
>

2021-12-12 14:13:42

[permalink] [raw]

Subject: Re: [PATCH v5 15/16] ima: Move dentries into ima_namespace

On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote:
> On Thu, Dec 09, 2021 at 02:38:13PM -0500, James Bottomley wrote:
[...]
> > @@ -317,21 +315,15 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink);
> > void securityfs_remove(struct dentry *dentry)
> > {
> > struct user_namespace *ns = dentry->d_sb->s_user_ns;
> > - struct inode *dir;
> >
> > if (!dentry || IS_ERR(dentry))
> > return;
> >
> > - dir = d_inode(dentry->d_parent);
> > - inode_lock(dir);
> > if (simple_positive(dentry)) {
> > - if (d_is_dir(dentry))
> > - simple_rmdir(dir, dentry);
> > - else
> > - simple_unlink(dir, dentry);
> > + d_delete(dentry);
>
> Not, that doesn't work. You can't just call d_delete() and dput() and
> even if I wouldn't advise it. And you also can't do this without
> taking the inode lock on the directory.

Agreed on that

> simple_rmdir()/simple_unlink() take care to update various inode
> fields in the parent dir and handle link counts. This really wants to
> be sm like
>
> struct inode *parent_inode;
>
> parent_inode = d_inode(dentry->d_parent);
> inode_lock(parent_inode);
> if (simple_positive(dentry)) {
> dget(dentry);
> if (d_is_dir(dentry)
> simple_unlink(parent_inode, dentry);
> else
> simple_unlink(parent_inode, dentry);
> d_delete(dentry);
> dput(dentry);
> }
> inode_unlock(parent_inode);

It just slightly annoys me how the simple_ functions change fields in
an inode that is about to be evicted ... it seems redundant; plus we
shouldn't care if the object we're deleting is a directory or file. I
also don't think we need the additional dget because the only consumer
is policy file removal and the opener of that file will have done a
dget. The inode lock now prevents us racing with another remove in the
case of two simultaneous writes.

How about

struct inode *parent_inode;

parent_inode = d_inode(dentry->d_parent);
inode_lock(parent_inode);
if (simple_positive(dentry)) {
drop_nlink(parent_inode);
d_delete(dentry);
dput(dentry);
}
inode_unlock(parent_inode);

James

2021-12-13 11:25:38