Re: [PATCH v4 16/16] ima: Setup securityfs for IMA namespace

On Wed, Dec 08, 2021 at 10:39:48AM -0500, Stefan Berger wrote:

On 12/8/21 07:58, Christian Brauner wrote:

quoted

On Tue, Dec 07, 2021 at 03:21:27PM -0500, Stefan Berger wrote:

quoted

  #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)

diff --git a/security/inode.c b/security/inode.c
index 121ac1874dde..10ee20917f42 100644
--- a/security/inode.c
+++ b/security/inode.c

@@ -16,6 +16,7 @@
  #include <linux/fs_context.h>
  #include <linux/mount.h>
  #include <linux/pagemap.h>
+#include <linux/ima.h>
  #include <linux/init.h>
  #include <linux/namei.h>
  #include <linux/security.h>

@@ -41,6 +42,7 @@ static const struct super_operations securityfs_super_operations = {
  static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
  {
  	static const struct tree_descr files[] = {{""}};
+	struct user_namespace *ns = fc->user_ns;
  	int error;
  	error = simple_fill_super(sb, SECURITYFS_MAGIC, files);

@@ -49,7 +51,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
  	sb->s_op = &securityfs_super_operations;
-	return 0;
+	if (ns != &init_user_ns)
+		error = ima_fs_ns_init(ns, sb->s_root);
+
+	return error;
  }
  static int securityfs_get_tree(struct fs_context *fc)

@@ -69,6 +74,11 @@ static int securityfs_init_fs_context(struct fs_context *fc)
  static void securityfs_kill_super(struct super_block *sb)
  {
+	struct user_namespace *ns = sb->s_fs_info;
+
+	if (ns != &init_user_ns)
+		ima_fs_ns_free_dentries(ns);

Say securityfs is unmounted. Then all the inodes and dentries become
invalid. It's not allowed to hold on to any dentries or inodes after the
super_block is shut down. So I just want to be sure that nothing in ima
can access these dentries after securityfs is unmounted.

quoted

To put it another way: why are they stored in struct ima_namespace in
the first place? If you don't pin a filesystem when creating files or
directories like you do for securityfs in init_ima_ns then you don't
need to hold on to them as they will be automatically be wiped during
umount.

The reason was so that securityfs for init_ima_ns and IMA namespaces could
share the code assigning to dentries to keep around and can clean up if an
error occurs while creating a dentry.

What about this: We keep the dentries in the ima_namespace, modify the code
creating the dentries in securityfs_create_dentry() to only take the
additional reference in case of init_user_ns (I suppose this is what you
suggest) and then keep 'static void ima_fs_ns_free_dentries()' only for
removing the dentries for the error case and never call it from
securityfs_kill_super()? Would that be acceptable?

If you create a range of dentries in fill_super post sb->s_root is
properly allocated and you fail in the middle you can simply return from
fill_super without bothering to clean them up as the vfs will
automatically clean those up when the dcache shrinker runs (If you've
increased the refcount as these functions do currently you need to
decrease it of course.).