Re: ima: use of radix tree cache indexing == massive waste of memory?

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Mon, 2010-10-18 at 02:25 -0400, Kyle McMartin wrote:
> If someone gives me a good reason why Fedora actually needs this
> enabled, I'm going to apply the following patch to our kernel so that
> IMA is globally an opt-in feature... Otherwise I'm inclined to just
> disable it.

Am hoping others will chime in.

> (But the more I look at this, the more it looks like a completely niche
>  option that has little use outside of three-letter agencies.)
> 
> I regret not looking at this more closely when it was enabled,
> (although, in my defence, when the option first showed up, I left it
> off...)
> 
> It's probably way more heavyweight than necessary, as I think in most
> cases the iint_initalized test will cover the call into IMA. But better
> safe than sorry or something and there's a bunch of other cases where
> -ENOMEM will leak back up from failing kmem_cache_alloc, iirc.
> 
> regards, Kyle

Thanks, will compile/test patch.

Mimi

> ---
> From: Kyle McMartin <kyle@mcmartin.ca>
> Date: Mon, 18 Oct 2010 02:08:35 -0400
> Subject: [PATCH] ima: allow it to be completely disabled (and default to off)
> 
> Allow IMA to be entirely disabled, don't even bother calling into
> the provided hooks, and avoid initializing caches.
> 
> (A lot of the hooks will test iint_initialized, and so this doubly
>  disables them, since the iint cache won't be enabled. But hey, we
>  avoid a pointless branch...)
> 
> Signed-off-by: Kyle McMartin <kyle@redhat.com>
> ---
>  include/linux/ima.h               |   66 +++++++++++++++++++++++++++++++++----
>  security/integrity/ima/ima_iint.c |   13 +++++--
>  security/integrity/ima/ima_main.c |   34 +++++++++++++------
>  3 files changed, 91 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 975837e..2fa456d 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -14,13 +14,65 @@
>  struct linux_binprm;
> 
>  #ifdef CONFIG_IMA
> -extern int ima_bprm_check(struct linux_binprm *bprm);
> -extern int ima_inode_alloc(struct inode *inode);
> -extern void ima_inode_free(struct inode *inode);
> -extern int ima_file_check(struct file *file, int mask);
> -extern void ima_file_free(struct file *file);
> -extern int ima_file_mmap(struct file *file, unsigned long prot);
> -extern void ima_counts_get(struct file *file);
> +
> +extern int ima_enabled;
> +
> +extern int __ima_bprm_check(struct linux_binprm *bprm);
> +extern int __ima_inode_alloc(struct inode *inode);
> +extern void __ima_inode_free(struct inode *inode);
> +extern int __ima_file_check(struct file *file, int mask);
> +extern void __ima_file_free(struct file *file);
> +extern int __ima_file_mmap(struct file *file, unsigned long prot);
> +extern void __ima_counts_get(struct file *file);
> +
> +static inline int ima_bprm_check(struct linux_binprm *bprm)
> +{
> +	if (ima_enabled)
> +		return __ima_bprm_check(bprm);
> +	return 0;
> +}
> +
> +static inline int ima_inode_alloc(struct inode *inode)
> +{
> +	if (ima_enabled)
> +		return __ima_inode_alloc(inode);
> +	return 0;
> +}
> +
> +static inline void ima_inode_free(struct inode *inode)
> +{
> +	if (ima_enabled)
> +		__ima_inode_free(inode);
> +	return;
> +}
> +
> +static inline int ima_file_check(struct file *file, int mask)
> +{
> +	if (ima_enabled)
> +		return __ima_file_check(file, mask);
> +	return 0;
> +}
> +
> +static inline void ima_file_free(struct file *file)
> +{
> +	if (ima_enabled)
> +		__ima_file_free(file);
> +	return;
> +}
> +
> +static inline int ima_file_mmap(struct file *file, unsigned long prot)
> +{
> +	if (ima_enabled)
> +		return __ima_file_mmap(file, prot);
> +	return 0;
> +}
> +
> +static inline void ima_counts_get(struct file *file)
> +{
> +	if (ima_enabled)
> +		return __ima_counts_get(file);
> +	return;
> +}
> 
>  #else
>  static inline int ima_bprm_check(struct linux_binprm *bprm)
> diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
> index afba4ae..767f026 100644
> --- a/security/integrity/ima/ima_iint.c
> +++ b/security/integrity/ima/ima_iint.c
> @@ -46,10 +46,10 @@ out:
>  }
> 
>  /**
> - * ima_inode_alloc - allocate an iint associated with an inode
> + * __ima_inode_alloc - allocate an iint associated with an inode
>   * @inode: pointer to the inode
>   */
> -int ima_inode_alloc(struct inode *inode)
> +int __ima_inode_alloc(struct inode *inode)
>  {
>  	struct ima_iint_cache *iint = NULL;
>  	int rc = 0;
> @@ -107,12 +107,12 @@ void iint_rcu_free(struct rcu_head *rcu_head)
>  }
> 
>  /**
> - * ima_inode_free - called on security_inode_free
> + * __ima_inode_free - called on security_inode_free
>   * @inode: pointer to the inode
>   *
>   * Free the integrity information(iint) associated with an inode.
>   */
> -void ima_inode_free(struct inode *inode)
> +void __ima_inode_free(struct inode *inode)
>  {
>  	struct ima_iint_cache *iint;
> 
> @@ -139,6 +139,11 @@ static void init_once(void *foo)
> 
>  static int __init ima_iintcache_init(void)
>  {
> +	extern int ima_enabled;
> +
> +	if (!ima_enabled)
> +		return 0;
> +
>  	iint_cache =
>  	    kmem_cache_create("iint_cache", sizeof(struct ima_iint_cache), 0,
>  			      SLAB_PANIC, init_once);
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index e662b89..92e084c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -26,6 +26,7 @@
>  #include "ima.h"
> 
>  int ima_initialized;
> +int ima_enabled = 0;
> 
>  char *ima_hash = "sha1";
>  static int __init hash_setup(char *str)
> @@ -36,6 +37,14 @@ static int __init hash_setup(char *str)
>  }
>  __setup("ima_hash=", hash_setup);
> 
> +static int __init ima_enable(char *str)
> +{
> +	if (strncmp(str, "on", 2) == 0)
> +		ima_enabled = 1;
> +	return 1;
> +}
> +__setup("ima=", ima_enable);
> +
>  struct ima_imbalance {
>  	struct hlist_node node;
>  	unsigned long fsmagic;
> @@ -130,7 +139,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
>  }
> 
>  /*
> - * ima_counts_get - increment file counts
> + * __ima_counts_get - increment file counts
>   *
>   * Maintain read/write counters for all files, but only
>   * invalidate the PCR for measured files:
> @@ -140,7 +149,7 @@ static void ima_inc_counts(struct ima_iint_cache *iint, fmode_t mode)
>   * 	  could result in a file measurement error.
>   *
>   */
> -void ima_counts_get(struct file *file)
> +void __ima_counts_get(struct file *file)
>  {
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct inode *inode = dentry->d_inode;
> @@ -204,13 +213,13 @@ static void ima_dec_counts(struct ima_iint_cache *iint, struct inode *inode,
>  }
> 
>  /**
> - * ima_file_free - called on __fput()
> + * __ima_file_free - called on __fput()
>   * @file: pointer to file structure being freed
>   *
>   * Flag files that changed, based on i_version;
>   * and decrement the iint readcount/writecount.
>   */
> -void ima_file_free(struct file *file)
> +void __ima_file_free(struct file *file)
>  {
>  	struct inode *inode = file->f_dentry->d_inode;
>  	struct ima_iint_cache *iint;
> @@ -255,7 +264,7 @@ out:
>  }
> 
>  /**
> - * ima_file_mmap - based on policy, collect/store measurement.
> + * __ima_file_mmap - based on policy, collect/store measurement.
>   * @file: pointer to the file to be measured (May be NULL)
>   * @prot: contains the protection that will be applied by the kernel.
>   *
> @@ -265,7 +274,7 @@ out:
>   * Return 0 on success, an error code on failure.
>   * (Based on the results of appraise_measurement().)
>   */
> -int ima_file_mmap(struct file *file, unsigned long prot)
> +int __ima_file_mmap(struct file *file, unsigned long prot)
>  {
>  	int rc;
> 
> @@ -278,7 +287,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
>  }
> 
>  /**
> - * ima_bprm_check - based on policy, collect/store measurement.
> + * __ima_bprm_check - based on policy, collect/store measurement.
>   * @bprm: contains the linux_binprm structure
>   *
>   * The OS protects against an executable file, already open for write,
> @@ -290,7 +299,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
>   * Return 0 on success, an error code on failure.
>   * (Based on the results of appraise_measurement().)
>   */
> -int ima_bprm_check(struct linux_binprm *bprm)
> +int __ima_bprm_check(struct linux_binprm *bprm)
>  {
>  	int rc;
> 
> @@ -300,7 +309,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
>  }
> 
>  /**
> - * ima_path_check - based on policy, collect/store measurement.
> + * __ima_path_check - based on policy, collect/store measurement.
>   * @file: pointer to the file to be measured
>   * @mask: contains MAY_READ, MAY_WRITE or MAY_EXECUTE
>   *
> @@ -309,7 +318,7 @@ int ima_bprm_check(struct linux_binprm *bprm)
>   * Always return 0 and audit dentry_open failures.
>   * (Return code will be based upon measurement appraisal.)
>   */
> -int ima_file_check(struct file *file, int mask)
> +int __ima_file_check(struct file *file, int mask)
>  {
>  	int rc;
> 
> @@ -318,12 +327,15 @@ int ima_file_check(struct file *file, int mask)
>  				 FILE_CHECK);
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(ima_file_check);
> +EXPORT_SYMBOL_GPL(__ima_file_check);
> 
>  static int __init init_ima(void)
>  {
>  	int error;
> 
> +	if (!ima_enabled)
> +		return 0;
> +
>  	error = ima_init();
>  	ima_initialized = 1;
>  	return error;