| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7e3f497e-fe51-bdc8-54c9-96b5e059935f@linux.ibm.com>
Date: Tue, 31 May 2022 15:26:21 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: "Serge E. Hallyn" <serge@...lyn.com>
Cc: linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
christian.brauner@...ntu.com, containers@...ts.linux.dev,
dmitry.kasatkin@...il.com, ebiederm@...ssion.com,
krzysztof.struczynski@...wei.com, roberto.sassu@...wei.com,
mpeters@...hat.com, lhinds@...hat.com, lsturman@...hat.com,
puiterwi@...hat.com, jejb@...ux.ibm.com, jamjoom@...ibm.com,
linux-kernel@...r.kernel.org, paul@...l-moore.com, rgb@...hat.com,
linux-security-module@...r.kernel.org, jmorris@...ei.org,
jpenumak@...hat.com,
James Bottomley <James.Bottomley@...senPartnership.com>,
Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v12 21/26] ima: Setup securityfs for IMA namespace
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@...ux.ibm.com>
>> Signed-off-by: James Bottomley <James.Bottomley@...senPartnership.com>
>> Acked-by: Christian Brauner <brauner@...nel.org>
>> Reviewed-by: Mimi Zohar <zohar@...ux.ibm.com>
>
> Acked-by: Serge Hallyn <serge@...lyn.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
>>
Powered by blists - more mailing lists