On 15-10-19 14:21:03, Mimi Zohar wrote:
> On Fri, 2015-10-16 at 22:31 +0300, Petko Manolov 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.
>
> There was no need for locking in the original version. This comment should
> be
> included in a change log to reflect different versions of the patch.
Yep, a message change is in order.
Petko
> > 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.
> > + * 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.
> > *
> > * 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) {
> > for (i = 0; i < MAX_LSM_RULES; i++) {
> > if (!entry->lsm[i].rule)
> > @@ -196,7 +195,6 @@ static void ima_lsm_update_rules(void)
> > BUG_ON(!entry->lsm[i].rule);
> > }
> > }
> > - mutex_unlock(&ima_rules_mutex);
> > }
> >
> > /**
> > @@ -319,9 +317,9 @@ static int get_subaction(struct ima_rule_entry *rule,
> > int func)
> > * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
> > * conditions.
> > *
> > - * (There is no need for locking when walking the policy list,
> > - * as elements in the list are never deleted, nor does the list
> > - * change.)
> > + * Since the IMA policy may be updated multiple times we need to lock the
> > + * list when walking it. Reads are many orders of magnitude more numerous
> > + * than writes so ima_match_policy() is classical RCU candidate.
> > */
> > int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
> > int flags)
> > @@ -329,7 +327,8 @@ int ima_match_policy(struct inode *inode, enum
> > ima_hooks func, int mask,
> > struct ima_rule_entry *entry;
> > int action = 0, actmask = flags | (flags << 1);
> >
> > - list_for_each_entry(entry, ima_rules, list) {
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(entry, ima_rules, list) {
> >
> > if (!(entry->action & actmask))
> > continue;
> > @@ -351,6 +350,7 @@ int ima_match_policy(struct inode *inode, enum
> > ima_hooks func, int mask,
> > if (!actmask)
> > break;
> > }
> > + rcu_read_unlock();
> >
> > return action;
> > }
> > @@ -365,7 +365,6 @@ void ima_update_policy_flag(void)
> > {
> > struct ima_rule_entry *entry;
> >
> > - ima_policy_flag = 0;
> > list_for_each_entry(entry, ima_rules, list) {
> > if (entry->action & IMA_DO_MASK)
> > ima_policy_flag |= entry->action;
> > @@ -419,12 +418,36 @@ void __init ima_init_policy(void)
> > * ima_update_policy - update default_rules with new measure rules
> > *
> > * Called on file .release to update the default rules with a complete new
> > - * policy. Once updated, the policy is locked, no additional rules can be
> > - * added to the policy.
> > + * policy. What we do here is to splice ima_policy_rules and
> > ima_temp_rules so
> > + * they make a queue. The policy may be updated multiple times and this
> > is the
> > + * RCU updater.
> > + *
> > + * Policy rules are never deleted so ima_policy_flag gets zeroed only once
> > when
> > + * we switch from the default policy to user defined.
> > */
> > void ima_update_policy(void)
> > {
> > - ima_rules = &ima_policy_rules;
> > + struct list_head *first, *last, *policy;
> > +
> > + /* append current policy with the new rules */
> > + first = (&ima_temp_rules)->next;
> > + last = (&ima_temp_rules)->prev;
> > + policy = &ima_policy_rules;
> > +
> > + synchronize_rcu();
> > +
> > + last->next = policy;
> > + rcu_assign_pointer(list_next_rcu(policy->prev), first);
> > + first->prev = policy->prev;
> > + policy->prev = last;
> > +
> > + /* prepare for the next policy rules addition */
> > + INIT_LIST_HEAD(&ima_temp_rules);
> > +
> > + if (ima_rules != policy) {
> > + ima_policy_flag = 0;
> > + ima_rules = policy;
> > + }
> > ima_update_policy_flag();
> > }
> >
> > @@ -746,7 +769,7 @@ static int ima_parse_rule(char *rule, struct
> > ima_rule_entry *entry)
> > * ima_parse_add_rule - add a rule to ima_policy_rules
> > * @rule - ima measurement policy rule
> > *
> > - * Uses a mutex to protect the policy list from multiple concurrent
> > writers.
> > + * Avoid locking by allowing just one writer at a time in
> > ima_write_policy()
> > * Returns the length of the rule parsed, an error code on failure
> > */
> > ssize_t ima_parse_add_rule(char *rule)
> > @@ -782,26 +805,27 @@ ssize_t ima_parse_add_rule(char *rule)
> > return result;
> > }
> >
> > - mutex_lock(&ima_rules_mutex);
> > - list_add_tail(&entry->list, &ima_policy_rules);
> > - mutex_unlock(&ima_rules_mutex);
> > + list_add_tail(&entry->list, &ima_temp_rules);
> >
> > return len;
> > }
> >
> > -/* ima_delete_rules called to cleanup invalid policy */
> > +/**
> > + * ima_delete_rules() called to cleanup invalid in-flight policy.
> > + * We don't need locking as we operate on the temp list, which is
> > + * different from the active one. There is also only one user of
> > + * ima_delete_rules() at a time.
> > + */
> > void ima_delete_rules(void)
> > {
> > struct ima_rule_entry *entry, *tmp;
> > int i;
> >
> > - mutex_lock(&ima_rules_mutex);
> > - list_for_each_entry_safe(entry, tmp, &ima_policy_rules, list) {
> > + list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
> > for (i = 0; i < MAX_LSM_RULES; i++)
> > kfree(entry->lsm[i].args_p);
> >
> > list_del(&entry->list);
> > kfree(entry);
> > }
> > - mutex_unlock(&ima_rules_mutex);
> > }
>
>
> --
> 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
--
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
