Re: ima: use of radix tree cache indexing == massive waste of memory?
From: Ryan Ware
Date: Mon Oct 18 2010 - 12:50:33 EST
On Sun, Oct 17, 2010 at 11:25 PM, Kyle McMartin <kyle@xxxxxxxxxxx> 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.
>
> (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.)
Not true. We'll be using it in MeeGo.
> 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.
Then let's bounce around ideas for making it more lightweight. I'd
love to hear suggestions.
Ryan
> regards, Kyle
>
> ---
> From: Kyle McMartin <kyle@xxxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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;
> --
> 1.7.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/