diff mbox series

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

Message ID 20190605083606.4209-2-janne.karhunen@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/2] LSM: switch to blocking policy update notifiers | expand

Commit Message

Janne Karhunen June 5, 2019, 8:36 a.m. UTC 
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(-)

Comments

Mimi Zohar June 6, 2019, 9:59 p.m. UTC | #1 
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;
>  	}
Mimi Zohar June 6, 2019, 10:28 p.m. UTC | #2 
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
diff mbox series

Patch

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