[PATCH 1/2] LSM: switch to blocking policy update notifiers

[PATCH 1/2] LSM: switch to blocking policy update notifiers

[Janne Karhunen]

Atomic policy updaters are not very useful as they cannot
usually perform the policy updates on their own. Since it
seems that there is no strict need for the atomicity,
switch to the blocking variant.

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 security/security.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/security.c b/security/security.c
index 23cbb1a295a3..c5e69ce81521 100644
--- a/security/security.c
+++ b/security/security.c
@@ -39,7 +39,7 @@
 #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
 
 struct security_hook_heads security_hook_heads __lsm_ro_after_init;
-static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
+static BLOCKING_NOTIFIER_HEAD(lsm_notifier_chain);
 
 static struct kmem_cache *lsm_file_cache;
 static struct kmem_cache *lsm_inode_cache;
@@ -432,19 +432,19 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
 
 int call_lsm_notifier(enum lsm_event event, void *data)
 {
-	return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
+	return blocking_notifier_call_chain(&lsm_notifier_chain, event, data);
 }
 EXPORT_SYMBOL(call_lsm_notifier);
 
 int register_lsm_notifier(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
+	return blocking_notifier_chain_register(&lsm_notifier_chain, nb);
 }
 EXPORT_SYMBOL(register_lsm_notifier);
 
 int unregister_lsm_notifier(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
+	return blocking_notifier_chain_unregister(&lsm_notifier_chain, nb);
 }
 EXPORT_SYMBOL(unregister_lsm_notifier);
 
-- 
2.17.1

[PATCH 2/2] ima: use the lsm policy update notifier

[Janne Karhunen]

Don't do lazy policy updates while running the rule matching,
run the updates as they happen.

Depends on commit cda44589be1c ("LSM: switch to blocking policy update notifiers")

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 security/integrity/ima/ima.h        |  2 ++
 security/integrity/ima/ima_main.c   |  8 ++++++
 security/integrity/ima/ima_policy.c | 44 +++++++++++++++++++++--------
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..2203451862d4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
 int __init ima_init_digests(void);
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+			  void *lsm_data);
 
 /*
  * used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 357edd140c09..f9629c5e1aee 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -43,6 +43,10 @@ int ima_appraise;
 int ima_hash_algo = HASH_ALGO_SHA1;
 static int hash_setup_done;
 
+static struct notifier_block ima_lsm_policy_notifier = {
+	.notifier_call = ima_lsm_policy_change,
+};
+
 static int __init hash_setup(char *str)
 {
 	struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -593,6 +597,10 @@ static int __init init_ima(void)
 		error = ima_init();
 	}
 
+	error = register_lsm_notifier(&ima_lsm_policy_notifier);
+	if (error)
+		pr_warn("Couldn't register LSM notifier, error %d\n", error);
+
 	if (!error)
 		ima_update_policy_flag();
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..4201a21ff42f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -252,12 +252,14 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
 /*
  * The LSM policy can be reloaded, leaving the IMA LSM based rules referring
  * to the old, stale LSM policy.  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.
+ * the reloaded LSM policy.
  */
 static void ima_lsm_update_rules(void)
 {
 	struct ima_rule_entry *entry;
+	void *rule_new;
+	char *lsm_new;
+	char *lsm_old;
 	int result;
 	int i;
 
@@ -265,15 +267,39 @@ static void ima_lsm_update_rules(void)
 		for (i = 0; i < MAX_LSM_RULES; i++) {
 			if (!entry->lsm[i].rule)
 				continue;
+
+			lsm_old = entry->lsm[i].args_p;
+			lsm_new = kstrdup(lsm_old, GFP_KERNEL);
+			if (unlikely(!lsm_new))
+				return;
+
 			result = security_filter_rule_init(entry->lsm[i].type,
 							   Audit_equal,
-							   entry->lsm[i].args_p,
-							   &entry->lsm[i].rule);
-			BUG_ON(!entry->lsm[i].rule);
+							   lsm_new,
+							   &rule_new);
+			if (result == -EINVAL)
+				pr_warn("ima: rule for LSM \'%d\' is invalid\n",
+					entry->lsm[i].type);
+
+			entry->lsm[i].rule = rule_new;
+			entry->lsm[i].args_p = lsm_new;
+			synchronize_rcu();
+
+			kfree(lsm_old);
 		}
 	}
 }
 
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+			  void *lsm_data)
+{
+	if (event != LSM_POLICY_CHANGE)
+		return NOTIFY_DONE;
+
+	ima_lsm_update_rules();
+	return NOTIFY_OK;
+}
+
 /**
  * ima_match_rules - determine whether an inode matches the measure rule.
  * @rule: a pointer to a rule
@@ -327,11 +353,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc = 0;
 		u32 osid;
-		int retried = 0;
 
 		if (!rule->lsm[i].rule)
 			continue;
-retry:
+
 		switch (i) {
 		case LSM_OBJ_USER:
 		case LSM_OBJ_ROLE:
@@ -352,11 +377,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		default:
 			break;
 		}
-		if ((rc < 0) && (!retried)) {
-			retried = 1;
-			ima_lsm_update_rules();
-			goto retry;
-		}
 		if (!rc)
 			return false;
 	}
-- 
2.17.1

Re: [PATCH 2/2] ima: use the lsm policy update notifier

[Stephen Smalley]

On 5/31/19 10:02 AM, Janne Karhunen wrote:
> Don't do lazy policy updates while running the rule matching,
> run the updates as they happen.
> 
> Depends on commit cda44589be1c ("LSM: switch to blocking policy update notifiers")
> 
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---
>   security/integrity/ima/ima.h        |  2 ++
>   security/integrity/ima/ima_main.c   |  8 ++++++
>   security/integrity/ima/ima_policy.c | 44 +++++++++++++++++++++--------
>   3 files changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d213e835c498..2203451862d4 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
>   int ima_init_template(void);
>   void ima_init_template_list(void);
>   int __init ima_init_digests(void);
> +int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> +			  void *lsm_data);
>   
>   /*
>    * used to protect h_table and sha_table
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 357edd140c09..f9629c5e1aee 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -43,6 +43,10 @@ int ima_appraise;
>   int ima_hash_algo = HASH_ALGO_SHA1;
>   static int hash_setup_done;
>   
> +static struct notifier_block ima_lsm_policy_notifier = {
> +	.notifier_call = ima_lsm_policy_change,
> +};
> +
>   static int __init hash_setup(char *str)
>   {
>   	struct ima_template_desc *template_desc = ima_template_desc_current();
> @@ -593,6 +597,10 @@ static int __init init_ima(void)
>   		error = ima_init();
>   	}
>   
> +	error = register_lsm_notifier(&ima_lsm_policy_notifier);
> +	if (error)
> +		pr_warn("Couldn't register LSM notifier, error %d\n", error);
> +
>   	if (!error)
>   		ima_update_policy_flag();
>   
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e0cc323f948f..4201a21ff42f 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -252,12 +252,14 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
>   /*
>    * The LSM policy can be reloaded, leaving the IMA LSM based rules referring
>    * to the old, stale LSM policy.  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.
> + * the reloaded LSM policy.
>    */
>   static void ima_lsm_update_rules(void)
>   {
>   	struct ima_rule_entry *entry;
> +	void *rule_new;
> +	char *lsm_new;
> +	char *lsm_old;
>   	int result;
>   	int i;
>   
> @@ -265,15 +267,39 @@ static void ima_lsm_update_rules(void)
>   		for (i = 0; i < MAX_LSM_RULES; i++) {
>   			if (!entry->lsm[i].rule)
>   				continue;
> +
> +			lsm_old = entry->lsm[i].args_p;
> +			lsm_new = kstrdup(lsm_old, GFP_KERNEL);
> +			if (unlikely(!lsm_new))
> +				return;
> +
>   			result = security_filter_rule_init(entry->lsm[i].type,
>   							   Audit_equal,
> -							   entry->lsm[i].args_p,
> -							   &entry->lsm[i].rule);
> -			BUG_ON(!entry->lsm[i].rule);
> +							   lsm_new,
> +							   &rule_new);
> +			if (result == -EINVAL)
> +				pr_warn("ima: rule for LSM \'%d\' is invalid\n",
> +					entry->lsm[i].type);
> +
> +			entry->lsm[i].rule = rule_new;

Doesn't this still leak the old entry->lsm[i].rule?

Also, I don't think you can just mutate entry like this under RCU. 
Don't you need to deep copy the entire entry, initialize the lsm rule in 
the new entry, switch the new entry for the old in the list via the 
appropriate rcu interface e.g. list_replace_rcu, and then free the old 
entry via call_rcu() or after synchronize_rcu()?  Consider how 
audit_update_lsm_rules() works; it takes a mutex to synchronize with 
other writers to the list and calls update_lsm_rule() on every rule. 
update_lsm_rule() checks whether the rule contains a lsm filter via 
security_audit_rule_known(), and if so, deep copies the rule via 
audit_dupe_rule(), replaces the old with the new via list_replace_rcu(), 
and then performs a delayed free of the old rule via call_rcu(). 
audit_dupe_rule() ultimately calls audit_dupe_lsm_field() to "duplicate" 
the LSM field information, which consists of copying the string and then 
calling security_audit_rule_init() aka security_filter_rule_init() to 
create the internal structure representation of the lsm rule from the 
string.  I could be wrong but I don't think you can short-circuit the 
copying or mutate entry in place like this.

> +			entry->lsm[i].args_p = lsm_new;
> +			synchronize_rcu();
> +
> +			kfree(lsm_old);
>   		}
>   	}
>   }
>   
> +int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> +			  void *lsm_data)
> +{
> +	if (event != LSM_POLICY_CHANGE)
> +		return NOTIFY_DONE;
> +
> +	ima_lsm_update_rules();
> +	return NOTIFY_OK;
> +}
> +
>   /**
>    * ima_match_rules - determine whether an inode matches the measure rule.
>    * @rule: a pointer to a rule
> @@ -327,11 +353,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>   	for (i = 0; i < MAX_LSM_RULES; i++) {
>   		int rc = 0;
>   		u32 osid;
> -		int retried = 0;
>   
>   		if (!rule->lsm[i].rule)
>   			continue;
> -retry:
> +
>   		switch (i) {
>   		case LSM_OBJ_USER:
>   		case LSM_OBJ_ROLE:
> @@ -352,11 +377,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>   		default:
>   			break;
>   		}
> -		if ((rc < 0) && (!retried)) {
> -			retried = 1;
> -			ima_lsm_update_rules();
> -			goto retry;
> -		}
>   		if (!rc)
>   			return false;
>   	}
> 

Re: [PATCH 2/2] ima: use the lsm policy update notifier

[Janne Karhunen]

On Fri, May 31, 2019 at 9:35 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:

> > +                     entry->lsm[i].rule = rule_new;
>
> Doesn't this still leak the old entry->lsm[i].rule?

Argh, clearly got a wrong understanding from different part of the
code. Will fix.


> Also, I don't think you can just mutate entry like this under RCU.

Yeah, it's definitely not the politically correct way of doing it.
Let's rework the entire list then, I will post another draft. It will
become somewhat more intrusive :-(


--
Janne

Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers

[Paul Moore]

On Fri, May 31, 2019 at 10:03 AM Janne Karhunen
<janne.karhunen@gmail.com> wrote:
> Atomic policy updaters are not very useful as they cannot
> usually perform the policy updates on their own. Since it
> seems that there is no strict need for the atomicity,
> switch to the blocking variant.
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---
>  security/security.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 23cbb1a295a3..c5e69ce81521 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -39,7 +39,7 @@
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> -static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> +static BLOCKING_NOTIFIER_HEAD(lsm_notifier_chain);
>
>  static struct kmem_cache *lsm_file_cache;
>  static struct kmem_cache *lsm_inode_cache;
> @@ -432,19 +432,19 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>
>  int call_lsm_notifier(enum lsm_event event, void *data)

As I mentioned in the other thread, I would like to see "blocking", or
similar, added to the lsm_notifier functions with this change.  It
makes it easier if/when we need to add both atomic and blocking
variants, as well as making it much more clear which version is being
used (helpful even now with just one variant).

For example: call_lsm_notifier() -> call_lsm_blocking_notifier(),
register_lsm_notifier() -> register_lsm_blocking_notifier().

>  {
> -       return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
> +       return blocking_notifier_call_chain(&lsm_notifier_chain, event, data);
>  }
>  EXPORT_SYMBOL(call_lsm_notifier);
>
>  int register_lsm_notifier(struct notifier_block *nb)
>  {
> -       return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
> +       return blocking_notifier_chain_register(&lsm_notifier_chain, nb);
>  }
>  EXPORT_SYMBOL(register_lsm_notifier);
>
>  int unregister_lsm_notifier(struct notifier_block *nb)
>  {
> -       return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
> +       return blocking_notifier_chain_unregister(&lsm_notifier_chain, nb);
>  }
>  EXPORT_SYMBOL(unregister_lsm_notifier);
>
> --
> 2.17.1
>


-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] ima: use the lsm policy update notifier

[Mimi Zohar]

Hi Janne,

One more comment below ...

> > +
> > +static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> > +{
> > +	struct ima_rule_entry *nentry;
> > +	int i, result;
> > +
> > +	nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
> > +	if (!nentry)
> > +		return NULL;
> > +
> > +	memcpy(nentry, entry, sizeof(*nentry));
> > +	nentry->fsname = NULL;
> > +	for (i = 0; i < MAX_LSM_RULES; i++) {
> > +		nentry->lsm[i].rule = NULL;
> > +		nentry->lsm[i].args_p = NULL;
> > +	}

I don't think this loop is necessary.  Either use kzalloc() or move
the initialization to inside the loop below.

> > +
> > +	if (entry->fsname) {
> > +		nentry->fsname = kstrdup(entry->fsname, GFP_KERNEL);
> > +		if (!nentry->fsname)
> > +			goto out_err;
> > +	}
> > +	for (i = 0; i < MAX_LSM_RULES; i++) {
> > +		if (!entry->lsm[i].rule)
> > +			continue;

To here.

> > +
> > +		nentry->lsm[i].type = entry->lsm[i].type;
> > +		nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
> > +						GFP_KERNEL);
> > +		if (!nentry->lsm[i].args_p)
> > +			goto out_err;

If the memory allocation fails, then nentry will be freed anyway.

thanks,

Mimid

Re: [PATCH 2/2] ima: use the lsm policy update notifier

[Mimi Zohar]

Hi Janne,

On Wed, 2019-06-05 at 11:36 +0300, Janne Karhunen wrote:
> Don't do lazy policy updates while running the rule matching,
> run the updates as they happen.
> 
> Depends on commit 2d1d5cee66d1 ("LSM: switch to blocking policy update notifiers")
> 
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>

Thanks!  Just a couple of minor things.  Comments inline below.

> ---
>  security/integrity/ima/ima.h        |   2 +
>  security/integrity/ima/ima_main.c   |   8 ++
>  security/integrity/ima/ima_policy.c | 124 +++++++++++++++++++++++-----
>  3 files changed, 114 insertions(+), 20 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d213e835c498..2203451862d4 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
>  int ima_init_template(void);
>  void ima_init_template_list(void);
>  int __init ima_init_digests(void);
> +int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> +			  void *lsm_data);
>  
>  /*
>   * used to protect h_table and sha_table
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index f16353b5097e..9e3ea8a3f2db 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -43,6 +43,10 @@ int ima_appraise;
>  int ima_hash_algo = HASH_ALGO_SHA1;
>  static int hash_setup_done;
>  
> +static struct notifier_block ima_lsm_policy_notifier = {
> +	.notifier_call = ima_lsm_policy_change,
> +};
> +
>  static int __init hash_setup(char *str)
>  {
>  	struct ima_template_desc *template_desc = ima_template_desc_current();
> @@ -621,6 +625,10 @@ static int __init init_ima(void)
>  		error = ima_init();
>  	}
>  
> +	error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
> +	if (error)
> +		pr_warn("Couldn't register LSM notifier, error %d\n", error);
> +
>  	if (!error)
>  		ima_update_policy_flag();
>  
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 1cc822a59054..7129dc4cd396 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -249,31 +249,121 @@ static int __init default_appraise_policy_setup(char *str)
>  }
>  __setup("ima_appraise_tcb", default_appraise_policy_setup);
>  
> +static void ima_lsm_free_rule(struct ima_rule_entry *entry)
> +{
> +	int i;
> +
> +	for (i = 0; i < MAX_LSM_RULES; i++) {
> +		kfree(entry->lsm[i].rule);
> +		kfree(entry->lsm[i].args_p);
> +	}
> +	kfree(entry->fsname);
> +	kfree(entry);
> +}

Matthew's patch, which adds per policy template format support, adds a
"template" field to entry.  In case anyone wants to backport this
patch, it might be simpler to make the change as a separate patch.


> +
> +static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> +{
> +	struct ima_rule_entry *nentry;
> +	int i, result;
> +
> +	nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
> +	if (!nentry)
> +		return NULL;
> +
> +	memcpy(nentry, entry, sizeof(*nentry));
> +	nentry->fsname = NULL;
> +	for (i = 0; i < MAX_LSM_RULES; i++) {
> +		nentry->lsm[i].rule = NULL;
> +		nentry->lsm[i].args_p = NULL;
> +	}
> +
> +	if (entry->fsname) {
> +		nentry->fsname = kstrdup(entry->fsname, GFP_KERNEL);
> +		if (!nentry->fsname)
> +			goto out_err;
> +	}
> +	for (i = 0; i < MAX_LSM_RULES; i++) {
> +		if (!entry->lsm[i].rule)
> +			continue;
> +
> +		nentry->lsm[i].type = entry->lsm[i].type;
> +		nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
> +						GFP_KERNEL);
> +		if (!nentry->lsm[i].args_p)
> +			goto out_err;
> +
> +		result = security_filter_rule_init(nentry->lsm[i].type,
> +						   Audit_equal,
> +						   nentry->lsm[i].args_p,
> +						   &nentry->lsm[i].rule);
> +		if (result == -EINVAL)
> +			pr_warn("ima: rule for LSM \'%d\' is invalid\n",
> +				entry->lsm[i].type);

If LSM labels can come and go, then perhaps instead of saying
"invalid" say "undefined" or "missing".


> +
> +	}
> +	return nentry;
> +
> +out_err:
> +	ima_lsm_free_rule(nentry);
> +	return NULL;
> +}
> +
> +static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> +{
> +	struct ima_rule_entry *nentry;
> +
> +	nentry = ima_lsm_copy_rule(entry);
> +	if (!nentry)
> +		return -ENOMEM;
> +
> +	list_replace_rcu(&entry->list, &nentry->list);
> +	synchronize_rcu();
> +	ima_lsm_free_rule(entry);
> +
> +	return 0;
> +}
> +
>  /*
>   * The LSM policy can be reloaded, leaving the IMA LSM based rules referring
>   * to the old, stale LSM policy.  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.
> + * the reloaded LSM policy.
>   */
>  static void ima_lsm_update_rules(void)
>  {
> -	struct ima_rule_entry *entry;
> -	int result;
> -	int i;
> +	struct ima_rule_entry *entry, *e;
> +	int i, result, needs_update;
>  
> -	list_for_each_entry(entry, &ima_policy_rules, list) {
> +	list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
> +		needs_update = 0;
>  		for (i = 0; i < MAX_LSM_RULES; i++) {
> -			if (!entry->lsm[i].rule)
> -				continue;
> -			result = security_filter_rule_init(entry->lsm[i].type,
> -							   Audit_equal,
> -							   entry->lsm[i].args_p,
> -							   &entry->lsm[i].rule);
> -			BUG_ON(!entry->lsm[i].rule);
> +			if (entry->lsm[i].rule) {
> +				needs_update = 1;
> +				break;
> +			}
> +		}
> +		if (!needs_update)
> +			continue;
> +
> +		result = ima_lsm_update_rule(entry);
> +		if (result) {
> +			pr_err("ima: lsm rule update error %d\n",
> +				result);

No need for separate line.

Mimi

> +			return;
>  		}
>  	}
>  }
>  
> +int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> +			  void *lsm_data)
> +{
> +	if (event != LSM_POLICY_CHANGE)
> +		return NOTIFY_DONE;
> +
> +	ima_lsm_update_rules();
> +	return NOTIFY_OK;
> +}
> +
>  /**
>   * ima_match_rules - determine whether an inode matches the measure rule.
>   * @rule: a pointer to a rule
> @@ -327,11 +417,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  	for (i = 0; i < MAX_LSM_RULES; i++) {
>  		int rc = 0;
>  		u32 osid;
> -		int retried = 0;
>  
>  		if (!rule->lsm[i].rule)
>  			continue;
> -retry:
> +
>  		switch (i) {
>  		case LSM_OBJ_USER:
>  		case LSM_OBJ_ROLE:
> @@ -352,11 +441,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  		default:
>  			break;
>  		}
> -		if ((rc < 0) && (!retried)) {
> -			retried = 1;
> -			ima_lsm_update_rules();
> -			goto retry;
> -		}
>  		if (!rc)
>  			return false;
>  	}

[PATCH 2/2] ima: use the lsm policy update notifier

[Janne Karhunen]

Don't do lazy policy updates while running the rule matching,
run the updates as they happen.

Depends on commit 2d1d5cee66d1 ("LSM: switch to blocking policy update notifiers")

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 security/integrity/ima/ima.h        |   2 +
 security/integrity/ima/ima_main.c   |   8 ++
 security/integrity/ima/ima_policy.c | 124 +++++++++++++++++++++++-----
 3 files changed, 114 insertions(+), 20 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..2203451862d4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
 int __init ima_init_digests(void);
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+			  void *lsm_data);
 
 /*
  * used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f16353b5097e..9e3ea8a3f2db 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -43,6 +43,10 @@ int ima_appraise;
 int ima_hash_algo = HASH_ALGO_SHA1;
 static int hash_setup_done;
 
+static struct notifier_block ima_lsm_policy_notifier = {
+	.notifier_call = ima_lsm_policy_change,
+};
+
 static int __init hash_setup(char *str)
 {
 	struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -621,6 +625,10 @@ static int __init init_ima(void)
 		error = ima_init();
 	}
 
+	error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
+	if (error)
+		pr_warn("Couldn't register LSM notifier, error %d\n", error);
+
 	if (!error)
 		ima_update_policy_flag();
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1cc822a59054..7129dc4cd396 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -249,31 +249,121 @@ static int __init default_appraise_policy_setup(char *str)
 }
 __setup("ima_appraise_tcb", default_appraise_policy_setup);
 
+static void ima_lsm_free_rule(struct ima_rule_entry *entry)
+{
+	int i;
+
+	for (i = 0; i < MAX_LSM_RULES; i++) {
+		kfree(entry->lsm[i].rule);
+		kfree(entry->lsm[i].args_p);
+	}
+	kfree(entry->fsname);
+	kfree(entry);
+}
+
+
+static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
+{
+	struct ima_rule_entry *nentry;
+	int i, result;
+
+	nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
+	if (!nentry)
+		return NULL;
+
+	memcpy(nentry, entry, sizeof(*nentry));
+	nentry->fsname = NULL;
+	for (i = 0; i < MAX_LSM_RULES; i++) {
+		nentry->lsm[i].rule = NULL;
+		nentry->lsm[i].args_p = NULL;
+	}
+
+	if (entry->fsname) {
+		nentry->fsname = kstrdup(entry->fsname, GFP_KERNEL);
+		if (!nentry->fsname)
+			goto out_err;
+	}
+	for (i = 0; i < MAX_LSM_RULES; i++) {
+		if (!entry->lsm[i].rule)
+			continue;
+
+		nentry->lsm[i].type = entry->lsm[i].type;
+		nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
+						GFP_KERNEL);
+		if (!nentry->lsm[i].args_p)
+			goto out_err;
+
+		result = security_filter_rule_init(nentry->lsm[i].type,
+						   Audit_equal,
+						   nentry->lsm[i].args_p,
+						   &nentry->lsm[i].rule);
+		if (result == -EINVAL)
+			pr_warn("ima: rule for LSM \'%d\' is invalid\n",
+				entry->lsm[i].type);
+
+	}
+	return nentry;
+
+out_err:
+	ima_lsm_free_rule(nentry);
+	return NULL;
+}
+
+static int ima_lsm_update_rule(struct ima_rule_entry *entry)
+{
+	struct ima_rule_entry *nentry;
+
+	nentry = ima_lsm_copy_rule(entry);
+	if (!nentry)
+		return -ENOMEM;
+
+	list_replace_rcu(&entry->list, &nentry->list);
+	synchronize_rcu();
+	ima_lsm_free_rule(entry);
+
+	return 0;
+}
+
 /*
  * The LSM policy can be reloaded, leaving the IMA LSM based rules referring
  * to the old, stale LSM policy.  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.
+ * the reloaded LSM policy.
  */
 static void ima_lsm_update_rules(void)
 {
-	struct ima_rule_entry *entry;
-	int result;
-	int i;
+	struct ima_rule_entry *entry, *e;
+	int i, result, needs_update;
 
-	list_for_each_entry(entry, &ima_policy_rules, list) {
+	list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
+		needs_update = 0;
 		for (i = 0; i < MAX_LSM_RULES; i++) {
-			if (!entry->lsm[i].rule)
-				continue;
-			result = security_filter_rule_init(entry->lsm[i].type,
-							   Audit_equal,
-							   entry->lsm[i].args_p,
-							   &entry->lsm[i].rule);
-			BUG_ON(!entry->lsm[i].rule);
+			if (entry->lsm[i].rule) {
+				needs_update = 1;
+				break;
+			}
+		}
+		if (!needs_update)
+			continue;
+
+		result = ima_lsm_update_rule(entry);
+		if (result) {
+			pr_err("ima: lsm rule update error %d\n",
+				result);
+			return;
 		}
 	}
 }
 
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
+			  void *lsm_data)
+{
+	if (event != LSM_POLICY_CHANGE)
+		return NOTIFY_DONE;
+
+	ima_lsm_update_rules();
+	return NOTIFY_OK;
+}
+
 /**
  * ima_match_rules - determine whether an inode matches the measure rule.
  * @rule: a pointer to a rule
@@ -327,11 +417,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc = 0;
 		u32 osid;
-		int retried = 0;
 
 		if (!rule->lsm[i].rule)
 			continue;
-retry:
+
 		switch (i) {
 		case LSM_OBJ_USER:
 		case LSM_OBJ_ROLE:
@@ -352,11 +441,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		default:
 			break;
 		}
-		if ((rc < 0) && (!retried)) {
-			retried = 1;
-			ima_lsm_update_rules();
-			goto retry;
-		}
 		if (!rc)
 			return false;
 	}
-- 
2.17.1