[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 at 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;
>   	}
>