Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <7e3f497e-fe51-bdc8-54c9-96b5e059935f@linux.ibm.com>
Date:   Tue, 31 May 2022 15:26:21 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH v12 21/26] ima: Setup securityfs for IMA namespace
Content-Language: en-US
To:     "Serge E. Hallyn" <serge@hallyn.com>
Cc:     linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        christian.brauner@ubuntu.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, jejb@linux.ibm.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,
        jpenumak@redhat.com,
        James Bottomley <James.Bottomley@HansenPartnership.com>,
        Christian Brauner <brauner@kernel.org>
References: <20220420140633.753772-1-stefanb@linux.ibm.com>
 <20220420140633.753772-22-stefanb@linux.ibm.com>
 <20220530011610.GA7909@mail.hallyn.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20220530011610.GA7909@mail.hallyn.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/29/22 21:16, Serge E. Hallyn wrote:
> On Wed, Apr 20, 2022 at 10:06:28AM -0400, Stefan Berger wrote:
>> Setup securityfs with symlinks, directories, and files for IMA
>> namespacing support. The same directory structure that IMA uses on the
>> host is also created for the namespacing case.
>>
>> The securityfs file and directory ownerships cannot be set when the
>> IMA namespace is initialized. Therefore, delay the setup of the file
>> system to a later point when securityfs is in securityfs_fill_super.
>>
>> Introduce a variable ima_policy_removed in ima_namespace that is used to
>> remember whether the policy file has previously been removed and thus
>> should not be created again in case of unmounting and again mounting
>> securityfs inside an IMA namespace.
>>
>> This filesystem can now be mounted as follows:
>>
>> mount -t securityfs /sys/kernel/security/ /sys/kernel/security/
>>
>> The following directories, symlinks, and files are available
>> when IMA namespacing is enabled, otherwise it will be empty:
>>
>> $ ls -l sys/kernel/security/
>> total 0
>> lr--r--r--. 1 root root 0 Dec  2 00:18 ima -> integrity/ima
>> drwxr-xr-x. 3 root root 0 Dec  2 00:18 integrity
>>
>> $ ls -l sys/kernel/security/ima/
>> total 0
>> -r--r-----. 1 root root 0 Dec  2 00:18 ascii_runtime_measurements
>> -r--r-----. 1 root root 0 Dec  2 00:18 binary_runtime_measurements
>> -rw-------. 1 root root 0 Dec  2 00:18 policy
>> -r--r-----. 1 root root 0 Dec  2 00:18 runtime_measurements_count
>> -r--r-----. 1 root root 0 Dec  2 00:18 violations
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Acked-by: Christian Brauner <brauner@kernel.org>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> Acked-by: Serge Hallyn <serge@hallyn.com>
> 
> but a nit below

Thanks for all the tags, I took all of them.
Would I need to worry about the other reviewerss' tags regardig the nit 
below? I'd rather leave it as-is otherwise.

    Stefan


> 
>>
>> ---
>>
>> v9:
>>   - rename policy_dentry_removed to ima_policy_removed
>> ---
>>   include/linux/ima.h             | 13 ++++++++++
>>   security/inode.c                |  6 ++++-
>>   security/integrity/ima/ima.h    |  1 +
>>   security/integrity/ima/ima_fs.c | 46 +++++++++++++++++++++++----------
>>   4 files changed, 52 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> index b9301e2aaa8b..0cbf0434bc93 100644
>> --- a/include/linux/ima.h
>> +++ b/include/linux/ima.h
>> @@ -41,6 +41,7 @@ extern int ima_measure_critical_data(const char *event_label,
>>   				     const char *event_name,
>>   				     const void *buf, size_t buf_len,
>>   				     bool hash, u8 *digest, size_t digest_len);
>> +extern int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root);
>>   
>>   #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>>   extern void ima_appraise_parse_cmdline(void);
>> @@ -227,6 +228,12 @@ void free_ima_ns(struct user_namespace *ns);
>>   
>>   void ima_free_ns_status_list(struct list_head *head, rwlock_t *ns_list_lock);
>>   
>> +static inline int ima_securityfs_init(struct user_namespace *user_ns,
>> +				      struct dentry *root)
>> +{
>> +	return ima_fs_ns_init(user_ns, root);
>> +}
>> +
>>   #else
>>   
>>   static inline void free_ima_ns(struct user_namespace *user_ns)
>> @@ -238,6 +245,12 @@ static inline void ima_free_ns_status_list(struct list_head *head,
>>   {
>>   }
>>   
>> +static inline int ima_securityfs_init(struct user_namespace *ns,
>> +				      struct dentry *root)
>> +{
>> +	return 0;
>> +}
>> +
>>   #endif /* CONFIG_IMA_NS */
>>   
>>   #endif /* _LINUX_IMA_H */
>> diff --git a/security/inode.c b/security/inode.c
>> index 84c9396792a9..e81f55f054dc 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>
>> @@ -82,7 +83,10 @@ static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
>>   	sb->s_op = &securityfs_super_operations;
>>   	sb->s_root->d_inode->i_op = &securityfs_dir_inode_operations;
>>   
>> -	return 0;
>> +	if (ns != &init_user_ns)
>> +		error = ima_securityfs_init(ns, sb->s_root);
>> +
>> +	return error;
>>   }
>>   
>>   static int securityfs_get_tree(struct fs_context *fc)
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index cb48fc1d5b80..801dc3c8bfde 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -152,6 +152,7 @@ struct ima_namespace {
>>   	int valid_policy;
>>   
>>   	struct dentry *ima_policy;
>> +	bool ima_policy_removed;
>>   
>>   	struct notifier_block ima_lsm_policy_notifier;
>>   } __randomize_layout;
>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>> index c41aa61b7393..84cd02a9e19b 100644
>> --- a/security/integrity/ima/ima_fs.c
>> +++ b/security/integrity/ima/ima_fs.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/rcupdate.h>
>>   #include <linux/parser.h>
>>   #include <linux/vmalloc.h>
>> +#include <linux/ima.h>
>>   
>>   #include "ima.h"
>>   
>> @@ -433,6 +434,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>>   #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
>>   	securityfs_remove(ns->ima_policy);
>>   	ns->ima_policy = NULL;
>> +	ns->ima_policy_removed = true;
>>   #elif defined(CONFIG_IMA_WRITE_POLICY)
>>   	clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
>>   #elif defined(CONFIG_IMA_READ_POLICY)
>> @@ -449,9 +451,11 @@ static const struct file_operations ima_measure_policy_ops = {
>>   	.llseek = generic_file_llseek,
>>   };
>>   
>> -static int __init ima_fs_ns_init(struct ima_namespace *ns)
>> +int ima_fs_ns_init(struct user_namespace *user_ns, struct dentry *root)
>>   {
>> -	struct dentry *ima_dir;
>> +	struct ima_namespace *ns = ima_ns_from_user_ns(user_ns);
>> +	struct dentry *int_dir;
>> +	struct dentry *ima_dir = NULL;
>>   	struct dentry *ima_symlink = NULL;
>>   	struct dentry *binary_runtime_measurements = NULL;
>>   	struct dentry *ascii_runtime_measurements = NULL;
>> @@ -459,11 +463,22 @@ static int __init ima_fs_ns_init(struct ima_namespace *ns)
>>   	struct dentry *violations = NULL;
>>   	int ret;
>>   
>> -	ima_dir = securityfs_create_dir("ima", integrity_dir);
>> -	if (IS_ERR(ima_dir))
>> -		return PTR_ERR(ima_dir);
>> +	/* FIXME: update when evm and integrity are namespaced */
>> +	if (user_ns != &init_user_ns) {
>> +		int_dir = securityfs_create_dir("integrity", root);
>> +		if (IS_ERR(int_dir))
>> +			return PTR_ERR(int_dir);
>> +	} else {
>> +		int_dir = integrity_dir;
>> +	}
>>   
>> -	ima_symlink = securityfs_create_symlink("ima", NULL, "integrity/ima",
>> +	ima_dir = securityfs_create_dir("ima", int_dir);
>> +	if (IS_ERR(ima_dir)) {
>> +		ret = PTR_ERR(ima_dir);
>> +		goto out;
>> +	}
>> +
>> +	ima_symlink = securityfs_create_symlink("ima", root, "integrity/ima",
>>   						NULL);
>>   	if (IS_ERR(ima_symlink)) {
>>   		ret = PTR_ERR(ima_symlink);
>> @@ -505,12 +520,15 @@ static int __init ima_fs_ns_init(struct ima_namespace *ns)
>>   		goto out;
>>   	}
>>   
>> -	ns->ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
>> -						ima_dir, NULL,
>> -						&ima_measure_policy_ops);
>> -	if (IS_ERR(ns->ima_policy)) {
>> -		ret = PTR_ERR(ns->ima_policy);
>> -		goto out;
>> +	if (!ns->ima_policy_removed) {
> 
> It would be nicer to avoid this indent level by just doing
> 
> 	if (ns->ima_policy_removed)
> 		return 0;
> 
> above the securityfs_create_file().
> 
>> +		ns->ima_policy =
>> +		    securityfs_create_file("policy", POLICY_FILE_FLAGS,
>> +					   ima_dir, NULL,
>> +					   &ima_measure_policy_ops);
>> +		if (IS_ERR(ns->ima_policy)) {
>> +			ret = PTR_ERR(ns->ima_policy);
>> +			goto out;
>> +		}
>>   	}
>>   
>>   	return 0;
>> @@ -522,11 +540,13 @@ static int __init ima_fs_ns_init(struct ima_namespace *ns)
>>   	securityfs_remove(binary_runtime_measurements);
>>   	securityfs_remove(ima_symlink);
>>   	securityfs_remove(ima_dir);
>> +	if (user_ns != &init_user_ns)
>> +		securityfs_remove(int_dir);
>>   
>>   	return ret;
>>   }
>>   
>>   int __init ima_fs_init(void)
>>   {
>> -	return ima_fs_ns_init(&init_ima_ns);
>> +	return ima_fs_ns_init(&init_user_ns, NULL);
>>   }
>> -- 
>> 2.34.1
>>