Message-ID: <edaa3e45-77aa-602d-2b30-66ef9a0a7161@schaufler-ca.com>
Date:   Tue, 7 Dec 2021 07:48:14 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.2
Subject: Re: [PATCH v3 00/16] ima: Namespace IMA with audit support in IMA-ns
Content-Language: en-US
To:     jejb@linux.ibm.com,
        Christian Brauner <christian.brauner@ubuntu.com>
Cc:     Stefan Berger <stefanb@linux.ibm.com>,
        linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        serge@hallyn.com, containers@lists.linux.dev,
        dmitry.kasatkin@gmail.com, ebiederm@xmission.com,
        krzysztof.struczynski@huawei.com, roberto.sassu@huawei.com,
        mpeters@redhat.com, lhinds@redhat.com, lsturman@redhat.com,
        puiterwi@redhat.com, jamjoom@us.ibm.com,
        linux-kernel@vger.kernel.org, paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org,
        Casey Schaufler <casey@schaufler-ca.com>
References: <20211206172600.1495968-1-stefanb@linux.ibm.com>
 <97ca7651b7ae9a0b6dce4d23c76af266fbd5642f.camel@linux.ibm.com>
 <20211207145901.awiibdgdidbshsbf@wittgenstein>
 <10fa531054c3b9e2a02ceb3dc007fa50e1bae1ff.camel@linux.ibm.com>
 <8491f76d5c8923f35216f55c030a68f478a0325a.camel@linux.ibm.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
In-Reply-To: <8491f76d5c8923f35216f55c030a68f478a0325a.camel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/7/2021 7:40 AM, James Bottomley wrote:
> On Tue, 2021-12-07 at 10:16 -0500, James Bottomley wrote:
>> On Tue, 2021-12-07 at 15:59 +0100, Christian Brauner wrote:
>>> On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley wrote:
> [...]
>>>>   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 reason for a notifier is that this current patch set only
>> namespaces ima, but we also have integrity and evm to do.  Plus, as
>> Casey said, we might get apparmour and selinux.  Since each of those
>> will also want to add entries in fill_super, the notifier mechanism
>> seemed fairly tailor made for this.  The alternative is to have a
>> load of
>>
>> #if CONFIG_securityfeature
>> callback()
>> #endif
>>
>> Inside securityfs_fill_super which is a bit inelegant.
>>
>>>   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.
>> I can thread the root as part of the callback.  I think I can still
>> use the standard securityfs calls because the only reason for the
>> dentry in the namespace is so the callee can pass NULL and have the
>> dentry created at the top level.  We can insist in the namespaced use
>> case that the callee always pass in the dentry, even for the top
>> level.
>>
>>> 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.)
>> Right, that's what the patch does.
>>
>>> We simply will expect users to add:
>>>
>>> ima_init_securityfs()
>>> mylsm_init_securityfs()
>> Yes, plus all the #ifdefs because securityfs can exist independently
>> of each of the features.  We can hide the ifdefs in the header files
>> and make the functions static do nothing if not defined, but the
>> ifdeffery has to live somewhere.
> Actually, I've got a much better reason: securityfs is a bool; all the
> other LSMs and IMA are tristates.  We can't call module init functions
> from core code, it has to be done by something like a notifier.

Err, no. LSMs are not available as loadable modules.