Re: [PATCH v4 01/16] ima: Add IMA namespace support

From: Stefan Berger <stefanb@linux.ibm.com>
To: Christian Brauner <christian.brauner@ubuntu.com>
Cc: 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, 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,
	James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: Re: [PATCH v4 01/16] ima: Add IMA namespace support
Date: Wed, 8 Dec 2021 09:50:15 -0500	[thread overview]
Message-ID: <5f9b38ef-04d7-e97e-be7d-424b39ef06bc@linux.ibm.com> (raw)
In-Reply-To: <20211208115456.nwhhdwub6zlcmzb3@wittgenstein>

On 12/8/21 06:54, Christian Brauner wrote:
> On Wed, Dec 08, 2021 at 12:29:18PM +0100, Christian Brauner wrote:
>> On Tue, Dec 07, 2021 at 03:21:12PM -0500, Stefan Berger wrote:
>>> Implement an IMA namespace data structure that gets created alongside a
>>> user namespace with CLONE_NEWUSER. This lays down the foundation for
>>> namespacing the different aspects of IMA (eg. IMA-audit, IMA-measurement,
>>> IMA-appraisal).
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>>> ---
>>>   include/linux/ima.h                      | 59 +++++++++++++++++
>>>   include/linux/user_namespace.h           |  4 ++
>>>   init/Kconfig                             | 10 +++
>>>   kernel/user.c                            |  9 ++-
>>>   kernel/user_namespace.c                  | 16 +++++
>>>   security/integrity/ima/Makefile          |  3 +-
>>>   security/integrity/ima/ima.h             |  4 ++
>>>   security/integrity/ima/ima_init.c        |  4 ++
>>>   security/integrity/ima/ima_init_ima_ns.c | 32 +++++++++
>>>   security/integrity/ima/ima_ns.c          | 82 ++++++++++++++++++++++++
>>>   10 files changed, 221 insertions(+), 2 deletions(-)
>>>   create mode 100644 security/integrity/ima/ima_init_ima_ns.c
>>>   create mode 100644 security/integrity/ima/ima_ns.c
>>>
>>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>>> index b6ab66a546ae..86d126b9ff2f 100644
>>> --- a/include/linux/ima.h
>>> +++ b/include/linux/ima.h
>>> @@ -11,6 +11,7 @@
>>>   #include <linux/fs.h>
>>>   #include <linux/security.h>
>>>   #include <linux/kexec.h>
>>> +#include <linux/user_namespace.h>
>>>   #include <crypto/hash_info.h>
>>>   struct linux_binprm;
>>>   
>>> @@ -210,6 +211,64 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
>>>   }
>>>   #endif /* CONFIG_IMA_APPRAISE */
>>>   
>>> +struct ima_namespace {
>>> +	struct kref kref;
>>> +	struct user_namespace *user_ns;
>>> +};
>>> +
>>> +extern struct ima_namespace init_ima_ns;
>>> +
>>> +#ifdef CONFIG_IMA_NS
>>> +
>>> +void free_ima_ns(struct kref *kref);
>>> +
>>> +static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
>>> +{
>>> +	if (ns)
>>> +		kref_get(&ns->kref);
>>> +
>>> +	return ns;
>>> +}
>>> +
>>> +static inline void put_ima_ns(struct ima_namespace *ns)
>>> +{
>>> +	if (ns) {
>>> +		pr_debug("DEREF   ima_ns: 0x%p  ctr: %d\n", ns, kref_read(&ns->kref));
>>> +		kref_put(&ns->kref, free_ima_ns);
>>> +	}
>>> +}
>>> +
>>> +struct ima_namespace *copy_ima_ns(struct ima_namespace *old_ns,
>>> +				  struct user_namespace *user_ns);
>>> +
>>> +static inline struct ima_namespace *get_current_ns(void)
>>> +{
>>> +	return current_user_ns()->ima_ns;
>>> +}
>>> +
>>> +#else
>>> +
>>> +static inline struct ima_namespace *get_ima_ns(struct ima_namespace *ns)
>>> +{
>>> +	return ns;
>>> +}
>>> +
>>> +static inline void put_ima_ns(struct ima_namespace *ns)
>>> +{
>>> +}
>>> +
>>> +static inline struct ima_namespace *copy_ima_ns(struct ima_namespace *old_ns,
>>> +						struct user_namespace *user_ns)
>>> +{
>>> +	return old_ns;
>>> +}
>>> +
>>> +static inline struct ima_namespace *get_current_ns(void)
>>> +{
>>> +	return &init_ima_ns;
>>> +}
>>> +#endif /* CONFIG_IMA_NS */
>>> +
>>>   #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
>>>   extern bool ima_appraise_signature(enum kernel_read_file_id func);
>>>   #else
>>> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
>>> index 33a4240e6a6f..5249db04d62b 100644
>>> --- a/include/linux/user_namespace.h
>>> +++ b/include/linux/user_namespace.h
>>> @@ -36,6 +36,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
>>>   #define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED
>>>   
>>>   struct ucounts;
>>> +struct ima_namespace;
>>>   
>>>   enum ucount_type {
>>>   	UCOUNT_USER_NAMESPACES,
>>> @@ -99,6 +100,9 @@ struct user_namespace {
>>>   #endif
>>>   	struct ucounts		*ucounts;
>>>   	long ucount_max[UCOUNT_COUNTS];
>>> +#ifdef CONFIG_IMA
>>> +	struct ima_namespace	*ima_ns;
>>> +#endif
>>>   } __randomize_layout;
>>>   
>>>   struct ucounts {
>>> diff --git a/init/Kconfig b/init/Kconfig
>>> index 11f8a845f259..27890607e8cb 100644
>>> --- a/init/Kconfig
>>> +++ b/init/Kconfig
>>> @@ -1242,6 +1242,16 @@ config NET_NS
>>>   	  Allow user space to create what appear to be multiple instances
>>>   	  of the network stack.
>>>   
>>> +config IMA_NS
>>> +	bool "IMA namespace"
>>> +	depends on USER_NS
>>> +	depends on IMA
>>> +	default y
>>> +	help
>>> +	  Allow the creation of IMA namespaces for each user namespace.
>>> +	  Namespaced IMA enables having IMA features work separately
>>> +	  in each IMA namespace.
>>> +
>>>   endif # NAMESPACES
>>>   
>>>   config CHECKPOINT_RESTORE
>>> diff --git a/kernel/user.c b/kernel/user.c
>>> index e2cf8c22b539..b5dc803a033d 100644
>>> --- a/kernel/user.c
>>> +++ b/kernel/user.c
>>> @@ -20,6 +20,10 @@
>>>   #include <linux/user_namespace.h>
>>>   #include <linux/proc_ns.h>
>>>   
>>> +#ifdef CONFIG_IMA
>>> +extern struct ima_namespace init_ima_ns;
>>> +#endif
>>> +
>>>   /*
>>>    * userns count is 1 for root user, 1 for init_uts_ns,
>>>    * and 1 for... ?
>>> @@ -55,7 +59,7 @@ struct user_namespace init_user_ns = {
>>>   			},
>>>   		},
>>>   	},
>>> -	.ns.count = REFCOUNT_INIT(3),
>>> +	.ns.count = REFCOUNT_INIT(4),
>>>   	.owner = GLOBAL_ROOT_UID,
>>>   	.group = GLOBAL_ROOT_GID,
>>>   	.ns.inum = PROC_USER_INIT_INO,
>>> @@ -67,6 +71,9 @@ struct user_namespace init_user_ns = {
>>>   	.keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list),
>>>   	.keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem),
>>>   #endif
>>> +#ifdef CONFIG_IMA
>>> +	.ima_ns = &init_ima_ns,
>>> +#endif
>>>   };
>>>   EXPORT_SYMBOL_GPL(init_user_ns);
>>>   
>>> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
>>> index 6b2e3ca7ee99..c26885343b19 100644
>>> --- a/kernel/user_namespace.c
>>> +++ b/kernel/user_namespace.c
>>> @@ -20,6 +20,7 @@
>>>   #include <linux/fs_struct.h>
>>>   #include <linux/bsearch.h>
>>>   #include <linux/sort.h>
>>> +#include <linux/ima.h>
>>>   
>>>   static struct kmem_cache *user_ns_cachep __read_mostly;
>>>   static DEFINE_MUTEX(userns_state_mutex);
>>> @@ -141,8 +142,20 @@ int create_user_ns(struct cred *new)
>>>   	if (!setup_userns_sysctls(ns))
>>>   		goto fail_keyring;
>>>   
>>> +#if CONFIG_IMA
>>> +	ns->ima_ns = copy_ima_ns(parent_ns->ima_ns, ns);
>>> +	if (IS_ERR(ns->ima_ns)) {
>>> +		ret = PTR_ERR(ns->ima_ns);
>>> +		goto fail_userns_sysctls;
>>> +	}
>>> +#endif
>>> +
>>>   	set_cred_user_ns(new, ns);
>>>   	return 0;
>>> +#if CONFIG_IMA
>>> +fail_userns_sysctls:
>>> +	retire_userns_sysctls(ns);
>>> +#endif
> If you rewrite copy_ima_ns() put_ima_ns() a little you can remove all
> the ifdefs in here and make this patch tiny.
>
> Make copy_ima_ns() return an int. Afaict, you can remove passing the
> parent ima_ns completely but if you have to pass through the
> parent_userns.
> Then you can initialize new_userns->ima_ns inside of copy_ima_ns() and
> the ifdef will only need to be visible inside copy_ima_ns() and not in
> create_user_ns(). Similar put_ima_ns() is only called in a single place
> namely kernel/user_namespace.c. Make it take a user_namespace argument
> and you can remove the ifdef in __put_user_ns() for put_ima_ns() too.
>
> So something -- COMPLETELY UNTESTED -- like the below should work:

Thanks. I did this now for v5.

    Stefan