Hi Petko, I have a question....
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; > You got rid of "ima_rules_mutex" in favor of RCU. I wonder why 'ima_write_mutex" is needed here? I do not see any other uses. ima_open_policy() provide "exclusive" access if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags)) return -EBUSY; > 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; > } > May be could actually just clear a flag and leave sysfs entry anyway.... #ifdef CONFIG_IMA_WRITE_POLICY clear_bit(IMA_FS_BUSY, &ima_fs_flags); #endif otherwise flag will be busy and next open fails as well... Dmitry > 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); > } > -- > 2.6.1 > -- Thanks, Dmitry -- 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