Re: [PATCH v3 00/16] ima: Namespace IMA with audit support in IMA-ns
From: Christian Brauner <hidden>
Date: 2021-12-07 14:59:17
Also in:
linux-integrity, lkml
On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley wrote:
On Mon, 2021-12-06 at 12:25 -0500, Stefan Berger wrote: [...]quoted
v3: - Further modifications to virtualized SecurityFS following James's posted patch - Dropping of early teardown for user_namespaces since not needed anymoreThis is my incremental to this series that moves the namespaced securityfs away from using a vfsmount and on to a root dentry instead, meaning we can call the blocking notifier from fill_super as Christian requested (and thus can remove the securityfs_notifier_sent indicator since it's only called once).
Somehow b4 retrieves your patch out-of-band which makes it weird to reply to so I'm copy-pasting it here and reply inline: On Mon, Dec 06, 2021 at 08:27:00PM +0000, James Bottomley wrote:
quoted hunk ↗ jump to hunk
--- include/linux/user_namespace.h | 3 +- security/inode.c | 55 +++++++++++++--------------------- 2 files changed, 22 insertions(+), 36 deletions(-)diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index 6b8bd060d8c4..03a0879376a0 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h@@ -104,8 +104,7 @@ struct user_namespace { struct ima_namespace *ima_ns; #endif #ifdef CONFIG_SECURITYFS - struct vfsmount *securityfs_mount; - bool securityfs_notifier_sent; + struct dentry *securityfs_root; #endif } __randomize_layout;diff --git a/security/inode.c b/security/inode.c index 45211845fc31..f8b6cb3dfb87 100644 --- a/security/inode.c +++ b/security/inode.c@@ -24,6 +24,7 @@ #include <linux/magic.h> #include <linux/user_namespace.h> +static struct vfsmount *securityfs_mount; static int securityfs_mount_count; static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier);@@ -40,43 +41,24 @@ static const struct super_operations securityfs_super_operations = { .free_inode = securityfs_free_inode, }; -static struct file_system_type fs_type; - -static void securityfs_free_context(struct fs_context *fc) -{ - struct user_namespace *ns = fc->user_ns; - - if (ns == &init_user_ns || - ns->securityfs_notifier_sent) - return; - - ns->securityfs_notifier_sent = true; - - ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT, - fs_type.name, NULL); - if (IS_ERR(ns->securityfs_mount)) { - printk(KERN_ERR "kern mount on securityfs ERROR: %ld\n", - PTR_ERR(ns->securityfs_mount)); - ns->securityfs_mount = NULL; - return; - } - - blocking_notifier_call_chain(&securityfs_ns_notifier, - SECURITYFS_NS_ADD, fc->user_ns); - mntput(ns->securityfs_mount); -} - static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc) { static const struct tree_descr files[] = {{""}}; int error; + struct user_namespace *ns = fc->user_ns; error = simple_fill_super(sb, SECURITYFS_MAGIC, files); if (error) return error; + ns->securityfs_root = dget(sb->s_root); + sb->s_op = &securityfs_super_operations; + if (ns != &init_user_ns) + blocking_notifier_call_chain(&securityfs_ns_notifier, + SECURITYFS_NS_ADD, ns);
I would propose not to use the notifier logic. While it might be nifty it's over-engineered in my opinion. The dentry stashing in struct user_namespace currently serves the purpose to make it retrievable in ima_fs_ns_init(). That doesn't justify its existence imho. There is one central place were all users of namespaced securityfs can create the files that they need to and that is in securityfs_fill_super(). (If you want to make that more obvious then give it a subdirectory securityfs and move inode.c in there.) We simply will expect users to add: ima_init_securityfs() mylsm_init_securityfs() that are to be placed in fill_super and ima_kill_securityfs() mylsm_kill_securityfs() that get called in kill_super and the root dentry and other relevant information should be passed explicitly into those functions. Then we can remove the dentry stashing from struct user_namespace altogether and the patch gets smaller too.