Re: [RFC 3/3] ima: make the integrity inode cache per namespace

["Serge E. Hallyn" <serge@hallyn.com>]

On Sat, Nov 27, 2021 at 04:45:49PM +0000, James Bottomley wrote:
> Currently we get one entry in the IMA log per unique file event.  So,
> if you have a measurement policy and it measures a particular binary
> it will not get measured again if it is subsequently executed. For
> Namespaced IMA, the correct behaviour seems to be to log once per
> inode per namespace (so every unique execution in a namespace gets a
> separate log entry).  Since logging once per inode per namespace is

I suspect I'll need to do a more in depth reading of the existing
code, but I'll ask the lazy question anyway (since you say "the
correct behavior seems to be") - is it actually important that
files which were appraised under a parent namespace's policy already
should be logged again?  Since you used the word "log" I'm assuming
this isn't building a running hash like a tpm pcr where skipping one
would invalidate rmeote attestation?

> different from current behaviour, it is only activated if the
> namespace appears in the log template (so there's no behaviour change
> for any of the original templates).
> 
> Expand the iint cache to have a list of namespaces, per iint entry,
> the inode has been seen in by moving the action flags and the
> measured_pcrs into a per-namespace structure.  The lifetime of these
> additional list entries is tied to the lifetime of the iint entry and
> the namespace, so if either is deleted, the new entry is.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  include/linux/ima.h                       |  13 ++-
>  include/linux/user_namespace.h            |   5 +
>  kernel/user.c                             |   1 +
>  kernel/user_namespace.c                   |   6 ++
>  security/integrity/iint.c                 |   4 +-
>  security/integrity/ima/Makefile           |   2 +-
>  security/integrity/ima/ima.h              |  21 +++-
>  security/integrity/ima/ima_api.c          |   7 +-
>  security/integrity/ima/ima_main.c         |  21 ++--
>  security/integrity/ima/ima_ns.c           | 115 ++++++++++++++++++++++
>  security/integrity/ima/ima_policy.c       |   2 +-
>  security/integrity/ima/ima_template_lib.c |   2 +
>  security/integrity/integrity.h            |  11 ++-
>  13 files changed, 192 insertions(+), 18 deletions(-)
>  create mode 100644 security/integrity/ima/ima_ns.c
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 09b14b73889e..dbbc0257d065 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -40,7 +40,8 @@ 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 void ima_init_user_ns(struct user_namespace *ns);
> +extern void ima_free_user_ns(struct user_namespace *ns);
>  #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>  extern void ima_appraise_parse_cmdline(void);
>  #else
> @@ -154,6 +155,16 @@ static inline int ima_measure_critical_data(const char *event_label,
>  	return -ENOENT;
>  }
>  
> +static inline void ima_init_user_ns(struct user_namespace *ns)
> +{
> +	return;
> +}
> +
> +static inline void ima_free_user_ns(struct user_namespace *ns)
> +{
> +	return;
> +}
> +
>  #endif /* CONFIG_IMA */
>  
>  #ifndef CONFIG_IMA_KEXEC
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index d155788abdc1..52968764b195 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -8,6 +8,7 @@
>  #include <linux/sched.h>
>  #include <linux/workqueue.h>
>  #include <linux/rwsem.h>
> +#include <linux/rwlock.h>
>  #include <linux/sysctl.h>
>  #include <linux/err.h>
>  #include <linux/uuid.h>
> @@ -101,6 +102,10 @@ struct user_namespace {
>  	struct ucounts		*ucounts;
>  	long ucount_max[UCOUNT_COUNTS];
>  	uuid_t			uuid;
> +#ifdef CONFIG_IMA
> +	struct list_head	ima_inode_list;
> +	rwlock_t		ima_inode_list_lock;
> +#endif
>  } __randomize_layout;
>  
>  struct ucounts {
> diff --git a/kernel/user.c b/kernel/user.c
> index bf9ae1d0b670..670d3c41c817 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -68,6 +68,7 @@ struct user_namespace init_user_ns = {
>  	.keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem),
>  #endif
>  	/* .uuid is initialized in user_namespaces_init() */
> +	/* all IMA fields are initialized by ima_init_user_ns() */
>  };
>  EXPORT_SYMBOL_GPL(init_user_ns);
>  
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 8ce57c16ddd3..8afa5dc0b992 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
>  #include <linux/export.h>
> +#include <linux/ima.h>
>  #include <linux/nsproxy.h>
>  #include <linux/slab.h>
>  #include <linux/sched/signal.h>
> @@ -144,6 +145,9 @@ int create_user_ns(struct cred *new)
>  	uuid_gen(&ns->uuid);
>  
>  	set_cred_user_ns(new, ns);
> +
> +	ima_init_user_ns(ns);
> +
>  	return 0;
>  fail_keyring:
>  #ifdef CONFIG_PERSISTENT_KEYRINGS
> @@ -200,6 +204,7 @@ static void free_user_ns(struct work_struct *work)
>  		}
>  		retire_userns_sysctls(ns);
>  		key_free_user_ns(ns);
> +		ima_free_user_ns(ns);
>  		ns_free_inum(&ns->ns);
>  		kmem_cache_free(user_ns_cachep, ns);
>  		dec_user_namespaces(ucounts);
> @@ -1389,6 +1394,7 @@ static __init int user_namespaces_init(void)
>  {
>  	user_ns_cachep = KMEM_CACHE(user_namespace, SLAB_PANIC | SLAB_ACCOUNT);
>  	uuid_gen(&init_user_ns.uuid);
> +	ima_init_user_ns(&init_user_ns);
>  	return 0;
>  }
>  subsys_initcall(user_namespaces_init);
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 8638976f7990..f714532feb7d 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -71,6 +71,7 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
>  static void iint_free(struct integrity_iint_cache *iint)
>  {
>  	kfree(iint->ima_hash);
> +	ima_ns_iint_list_free(iint);
>  	iint->ima_hash = NULL;
>  	iint->version = 0;
>  	iint->flags = 0UL;
> @@ -81,7 +82,7 @@ static void iint_free(struct integrity_iint_cache *iint)
>  	iint->ima_read_status = INTEGRITY_UNKNOWN;
>  	iint->ima_creds_status = INTEGRITY_UNKNOWN;
>  	iint->evm_status = INTEGRITY_UNKNOWN;
> -	iint->measured_pcrs = 0;
> +	INIT_LIST_HEAD(&iint->ns_list);
>  	kmem_cache_free(iint_cache, iint);
>  }
>  
> @@ -170,6 +171,7 @@ static void init_once(void *foo)
>  	iint->ima_creds_status = INTEGRITY_UNKNOWN;
>  	iint->evm_status = INTEGRITY_UNKNOWN;
>  	mutex_init(&iint->mutex);
> +	INIT_LIST_HEAD(&iint->ns_list);
>  }
>  
>  static int __init integrity_iintcache_init(void)
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index 2499f2485c04..1741aa5d97bc 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -7,7 +7,7 @@
>  obj-$(CONFIG_IMA) += ima.o
>  
>  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> -	 ima_policy.o ima_template.o ima_template_lib.o
> +	 ima_policy.o ima_template.o ima_template_lib.o ima_ns.o
>  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
>  ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
>  ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index be965a8715e4..047256be7195 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -119,6 +119,16 @@ struct ima_kexec_hdr {
>  	u64 count;
>  };
>  
> +/* IMA cache of per-user-namespace flags */
> +struct ima_ns_cache {
> +	struct user_namespace *ns;
> +	struct integrity_iint_cache *iint;
> +	struct list_head ns_list;
> +	struct list_head iint_list;
> +	unsigned long measured_pcrs;
> +	unsigned long flags;
> +};
> +
>  extern const int read_idmap[];
>  
>  #ifdef CONFIG_HAVE_IMA_KEXEC
> @@ -263,7 +273,8 @@ int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
>  int ima_collect_measurement(struct integrity_iint_cache *iint,
>  			    struct file *file, void *buf, loff_t size,
>  			    enum hash_algo algo, struct modsig *modsig);
> -void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
> +void ima_store_measurement(struct integrity_iint_cache *iint,
> +			   struct ima_ns_cache *nsc, struct file *file,
>  			   const unsigned char *filename,
>  			   struct evm_ima_xattr_data *xattr_value,
>  			   int xattr_len, const struct modsig *modsig, int pcr,
> @@ -301,6 +312,14 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
>  void ima_policy_stop(struct seq_file *m, void *v);
>  int ima_policy_show(struct seq_file *m, void *v);
>  
> +/* IMA Namespace related functions */
> +extern bool ima_ns_in_template;
> +struct ima_ns_cache *ima_ns_cache_get(struct integrity_iint_cache *iint,
> +				      struct user_namespace *ns);
> +void ima_ns_cache_clear(struct integrity_iint_cache *iint);
> +void ima_init_ns(void);
> +
> +
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE	0x01
>  #define IMA_APPRAISE_FIX	0x02
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index a64fb0130b01..d613ea1ee378 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -298,6 +298,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   * Must be called with iint->mutex held.
>   */
>  void ima_store_measurement(struct integrity_iint_cache *iint,
> +			   struct ima_ns_cache *nsc,
>  			   struct file *file, const unsigned char *filename,
>  			   struct evm_ima_xattr_data *xattr_value,
>  			   int xattr_len, const struct modsig *modsig, int pcr,
> @@ -322,7 +323,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
>  	 * appraisal, but a file measurement from earlier might already exist in
>  	 * the measurement list.
>  	 */
> -	if (iint->measured_pcrs & (0x1 << pcr) && !modsig)
> +	if (nsc->measured_pcrs & (0x1 << pcr) && !modsig)
>  		return;
>  
>  	result = ima_alloc_init_template(&event_data, &entry, template_desc);
> @@ -334,8 +335,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint,
>  
>  	result = ima_store_template(entry, violation, inode, filename, pcr);
>  	if ((!result || result == -EEXIST) && !(file->f_flags & O_DIRECT)) {
> -		iint->flags |= IMA_MEASURED;
> -		iint->measured_pcrs |= (0x1 << pcr);
> +		nsc->flags |= IMA_MEASURED;
> +		nsc->measured_pcrs |= (0x1 << pcr);
>  	}
>  	if (result < 0)
>  		ima_free_template_entry(entry);
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 465865412100..049710203fac 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -168,8 +168,8 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>  		if (!IS_I_VERSION(inode) ||
>  		    !inode_eq_iversion(inode, iint->version) ||
>  		    (iint->flags & IMA_NEW_FILE)) {
> -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> -			iint->measured_pcrs = 0;
> +			iint->flags &= ~IMA_NEW_FILE;
> +			ima_ns_cache_clear(iint);
>  			if (update)
>  				ima_update_xattr(iint, file);
>  		}
> @@ -204,6 +204,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  {
>  	struct inode *inode = file_inode(file);
>  	struct integrity_iint_cache *iint = NULL;
> +	struct ima_ns_cache *nsc = NULL;
>  	struct ima_template_desc *template_desc = NULL;
>  	char *pathbuf = NULL;
>  	char filename[NAME_MAX];
> @@ -274,20 +275,20 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  	    ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
>  	     !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) &&
>  	     !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) {
> -		iint->flags &= ~IMA_DONE_MASK;
> -		iint->measured_pcrs = 0;
> +		ima_ns_cache_clear(iint);
>  	}
> +	nsc = ima_ns_cache_get(iint, current_user_ns());
>  
>  	/* Determine if already appraised/measured based on bitmask
>  	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
>  	 *  IMA_AUDIT, IMA_AUDITED)
>  	 */
> -	iint->flags |= action;
> +	nsc->flags |= action;
>  	action &= IMA_DO_MASK;
> -	action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
> +	action &= ~((nsc->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
>  
>  	/* If target pcr is already measured, unset IMA_MEASURE action */
> -	if ((action & IMA_MEASURE) && (iint->measured_pcrs & (0x1 << pcr)))
> +	if ((action & IMA_MEASURE) && (nsc->measured_pcrs & (0x1 << pcr)))
>  		action ^= IMA_MEASURE;
>  
>  	/* HASH sets the digital signature and update flags, nothing else */
> @@ -297,7 +298,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  		if ((xattr_value && xattr_len > 2) &&
>  		    (xattr_value->type == EVM_IMA_XATTR_DIGSIG))
>  			set_bit(IMA_DIGSIG, &iint->atomic_flags);
> -		iint->flags |= IMA_HASHED;
> +		nsc->flags |= IMA_HASHED;
>  		action ^= IMA_HASH;
>  		set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
>  	}
> @@ -342,7 +343,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
>  
>  	if (action & IMA_MEASURE)
> -		ima_store_measurement(iint, file, pathname,
> +		ima_store_measurement(iint, nsc, file, pathname,
>  				      xattr_value, xattr_len, modsig, pcr,
>  				      template_desc);
>  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
> @@ -1047,6 +1048,8 @@ static int __init init_ima(void)
>  	if (error)
>  		return error;
>  
> +	ima_init_ns();
> +
>  	error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
>  	if (error)
>  		pr_warn("Couldn't register LSM notifier, error %d\n", error);
> diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
> new file mode 100644
> index 000000000000..7134ba7f3544
> --- /dev/null
> +++ b/security/integrity/ima/ima_ns.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * IMA Namespace Support routines
> + */
> +
> +#include <linux/ima.h>
> +#include <linux/user_namespace.h>
> +
> +#include "ima.h"
> +
> +static struct kmem_cache *ns_cache __read_mostly;
> +bool ima_ns_in_template __read_mostly;
> +
> +void __init ima_init_ns(void)
> +{
> +	ns_cache = KMEM_CACHE(ima_ns_cache, SLAB_PANIC);
> +}
> +
> +void ima_init_user_ns(struct user_namespace *ns)
> +{
> +	INIT_LIST_HEAD(&ns->ima_inode_list);
> +}
> +
> +void ima_free_user_ns(struct user_namespace *ns)
> +{
> +	struct ima_ns_cache *entry;
> +
> +	/* no refs to ns left, so no need to lock */
> +	while (!list_empty(&ns->ima_inode_list)) {
> +		entry = list_entry(ns->ima_inode_list.next, struct ima_ns_cache,
> +				   ns_list);
> +
> +		/* iint cache entry is still active to lock to delete */
> +		write_lock(&entry->iint->ns_list_lock);
> +		list_del(&entry->iint_list);
> +		write_unlock(&entry->iint->ns_list_lock);
> +
> +		list_del(&entry->ns_list);
> +		kmem_cache_free(ns_cache, entry);
> +	}
> +}
> +
> +struct ima_ns_cache *ima_ns_cache_get(struct integrity_iint_cache *iint,
> +				      struct user_namespace *ns)
> +{
> +	struct ima_ns_cache *entry = NULL;
> +
> +	if (!ima_ns_in_template)
> +		/*
> +		 * if we're not logging the namespace, don't separate the
> +		 * iint cache per namespace.  This preserves original
> +		 * behaviour for the non-ns case.
> +		 */
> +		ns = &init_user_ns;
> +
> +	read_lock(&iint->ns_list_lock);
> +	list_for_each_entry(entry, &iint->ns_list, ns_list)
> +		if (entry->ns == ns)
> +			break;
> +	read_unlock(&iint->ns_list_lock);
> +
> +	if (entry && entry->ns == ns)
> +		return entry;
> +
> +	entry = kmem_cache_zalloc(ns_cache, GFP_NOFS);
> +	if (!entry)
> +		return NULL;
> +
> +	/* no refs taken: entry is freed on either ns delete or iint delete */
> +	entry->ns = ns;
> +	entry->iint = iint;
> +
> +	write_lock(&iint->ns_list_lock);
> +	list_add_tail(&entry->iint_list, &iint->ns_list);
> +	write_unlock(&iint->ns_list_lock);
> +
> +	write_lock(&ns->ima_inode_list_lock);
> +	list_add_tail(&entry->ns_list, &ns->ima_inode_list);
> +	write_unlock(&ns->ima_inode_list_lock);
> +
> +	return entry;
> +}
> +
> +/* clear the flags and measured PCR for every entry in the iint  */
> +void ima_ns_cache_clear(struct integrity_iint_cache *iint)
> +{
> +	struct ima_ns_cache *entry;
> +
> +	read_lock(&iint->ns_list_lock);
> +	list_for_each_entry(entry, &iint->ns_list, ns_list) {
> +		entry->flags = 0;
> +		entry->measured_pcrs = 0;
> +	}
> +	read_unlock(&iint->ns_list_lock);
> +}
> +
> +void ima_ns_iint_list_free(struct integrity_iint_cache *iint)
> +{
> +	struct ima_ns_cache *entry;
> +
> +	/* iint locking unnecessary; no-one should have acces to the list */
> +	while (!list_empty(&iint->ns_list)) {
> +		entry = list_entry(iint->ns_list.next, struct ima_ns_cache,
> +				   iint_list);
> +
> +		/* namespace is still active to lock to delete */
> +		write_lock(&entry->ns->ima_inode_list_lock);
> +		list_del(&entry->ns_list);
> +		write_unlock(&entry->ns->ima_inode_list_lock);
> +
> +		list_del(&entry->iint_list);
> +		kmem_cache_free(ns_cache, entry);
> +	}
> +}
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 320ca80aacab..9434a1064da6 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -50,7 +50,7 @@
>  #define DONT_HASH	0x0200
>  
>  #define INVALID_PCR(a) (((a) < 0) || \
> -	(a) >= (sizeof_field(struct integrity_iint_cache, measured_pcrs) * 8))
> +	(a) >= (sizeof_field(struct ima_ns_cache, measured_pcrs) * 8))
>  
>  int ima_policy_flag;
>  static int temp_ima_appraise;
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index ebd54c1b5206..d1a381add127 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -703,6 +703,8 @@ int ima_eventinodexattrvalues_init(struct ima_event_data *event_data,
>  int ima_ns_init(struct ima_event_data *event_data,
>  		struct ima_field_data *field_data)
>  {
> +	if (unlikely(!ima_ns_in_template))
> +		ima_ns_in_template = true;
>  	return ima_write_template_field_data(&current_user_ns()->uuid,
>  					     UUID_SIZE, DATA_FMT_UUID,
>  					     field_data);
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 547425c20e11..8755635da3ff 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -129,7 +129,6 @@ struct integrity_iint_cache {
>  	struct inode *inode;	/* back pointer to inode in question */
>  	u64 version;		/* track inode changes */
>  	unsigned long flags;
> -	unsigned long measured_pcrs;
>  	unsigned long atomic_flags;
>  	enum integrity_status ima_file_status:4;
>  	enum integrity_status ima_mmap_status:4;
> @@ -138,6 +137,8 @@ struct integrity_iint_cache {
>  	enum integrity_status ima_creds_status:4;
>  	enum integrity_status evm_status:4;
>  	struct ima_digest_data *ima_hash;
> +	struct list_head ns_list; /* list of namespaces inode seen in */
> +	rwlock_t ns_list_lock;
>  };
>  
>  /* rbtree tree calls to lookup, insert, delete
> @@ -225,6 +226,14 @@ static inline void ima_load_x509(void)
>  }
>  #endif
>  
> +#ifdef CONFIG_IMA
> +void ima_ns_iint_list_free(struct integrity_iint_cache *iint);
> +#else
> +void ima_ns_iint_list_free(struct integrity_iint_cache *iint)
> +{
> +}
> +#endif
> +
>  #ifdef CONFIG_EVM_LOAD_X509
>  void __init evm_load_x509(void);
>  #else
> -- 
> 2.33.0