On Sat, 2015-10-24 at 17:06 +0300, Dmitry Kasatkin wrote:
> Hi,
>
> On Fri, Oct 16, 2015 at 10:31 PM, Petko Manolov <pet...@mip-labs.com> wrote:
> > IMA policy can now be updated multiple times. The new rules get appended
> > to the original policy. Have in mind that the rules are scanned in FIFO
> > order so be careful when you add new ones.
> >
> > The mutex locks are replaced with RCU, which should lead to faster policy
> > traversals. The new rules are first appended to a temporary list, which
> > on error gets released without disturbing the normal IMA operations.
> >
> > Signed-off-by: Petko Manolov <pet...@mip-labs.com>
> > ---
> > security/integrity/ima/Kconfig | 14 ++++++++
> > security/integrity/ima/ima_fs.c | 13 +++++++
> > security/integrity/ima/ima_policy.c | 70
> > +++++++++++++++++++++++++------------
> > 3 files changed, 74 insertions(+), 23 deletions(-)
> >
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index df30334..15264b7 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -107,6 +107,20 @@ config IMA_DEFAULT_HASH
> > default "sha512" if IMA_DEFAULT_HASH_SHA512
> > default "wp512" if IMA_DEFAULT_HASH_WP512
> >
> > +config IMA_WRITE_POLICY
> > + bool "Enable multiple writes to the IMA policy"
> > + depends on IMA
> > + default n
> > + help
> > + IMA policy can now be updated multiple times. The new rules get
> > + appended to the original policy. Have in mind that the rules are
> > + scanned in FIFO order so be careful when you add new ones.
> > +
> > + WARNING: Potential security hole - should be used with care in
> > + production-grade kernels!
> > +
> > + If unsure, say N.
> > +
> > config IMA_APPRAISE
> > bool "Appraise integrity measurements"
> > depends on IMA
> > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c
> > index 816d175..a3cf5c0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -25,6 +25,8 @@
> >
> > #include "ima.h"
> >
> > +static DEFINE_MUTEX(ima_write_mutex);
> > +
> > static int valid_policy = 1;
> > #define TMPBUFLEN 12
> > static ssize_t ima_show_htable_value(char __user *buf, size_t count,
> > @@ -261,6 +263,11 @@ static ssize_t ima_write_policy(struct file *file,
> > const char __user *buf,
> > {
> > char *data = NULL;
> > ssize_t result;
> > + int res;
> > +
> > + res = mutex_lock_interruptible(&ima_write_mutex);
> > + if (res)
> > + return res;
> >
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> > @@ -286,6 +293,8 @@ out:
> > if (result < 0)
> > valid_policy = 0;
> > kfree(data);
> > + mutex_unlock(&ima_write_mutex);
> > +
> > return result;
> > }
> >
> > @@ -337,8 +346,12 @@ static int ima_release_policy(struct inode *inode,
> > struct file *file)
> > return 0;
> > }
> > ima_update_policy();
> > +#ifndef CONFIG_IMA_WRITE_POLICY
> > securityfs_remove(ima_policy);
> > ima_policy = NULL;
> > +#else
> > + clear_bit(IMA_FS_BUSY, &ima_fs_flags);
> > +#endif
> > return 0;
> > }
> >
> > diff --git a/security/integrity/ima/ima_policy.c
> > b/security/integrity/ima/ima_policy.c
> > index 3997e20..7ace4e4 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -16,6 +16,7 @@
> > #include <linux/magic.h>
> > #include <linux/parser.h>
> > #include <linux/slab.h>
> > +#include <linux/rculist.h>
> > #include <linux/genhd.h>
> >
> > #include "ima.h"
> > @@ -135,11 +136,11 @@ static struct ima_rule_entry default_appraise_rules[]
> > = {
> >
> > static LIST_HEAD(ima_default_rules);
> > static LIST_HEAD(ima_policy_rules);
> > +static LIST_HEAD(ima_temp_rules);
> > static struct list_head *ima_rules;
> >
> > -static DEFINE_MUTEX(ima_rules_mutex);
> > -
> > static int ima_policy __initdata;
> > +
> > static int __init default_measure_policy_setup(char *str)
> > {
> > if (ima_policy)
> > @@ -171,9 +172,8 @@ static int __init default_appraise_policy_setup(char
> > *str)
> > __setup("ima_appraise_tcb", default_appraise_policy_setup);
> >
> > /*
> > - * Although the IMA policy does not change, the LSM policy can be
> > - * reloaded, leaving the IMA LSM based rules referring to the old,
> > - * stale LSM policy.
Please don't remove this comment.
> > + * Blocking here is not legal as we hold an RCU lock. ima_update_policy()
> > + * should make it safe to walk the list at any time.
> >
I'm not sure this comment is needed. If it is needed, this should be
moved to below the function description.
> > * Update the IMA LSM based rules to reflect the reloaded LSM policy.
> > * We assume the rules still exist; and BUG_ON() if they don't.
> > @@ -184,7 +184,6 @@ static void ima_lsm_update_rules(void)
> > int result;
> > int i;
> >
> > - mutex_lock(&ima_rules_mutex);
> > list_for_each_entry_safe(entry, tmp, &ima_policy_rules, list) {
>
>
> Use of "list_for_each_entry_safe" makes me having a doubt.
>
> "safe" versions are used when entries are removed while walk.
>
> If it is so, then entire RCU case is impossible?
Taking the lock here was to prevent modifying the LSM rule number
multiple times. Using the "safe" version was never needed. The worst
that can happen here is that the LSM rule number is updated multiple
times.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
