ima: ima/lsm policy rule loading logic bug fixes

Commit Message

Janne Karhunen Jan. 9, 2020, 2:08 p.m. UTC

Keep the ima policy rules around from the beginning even
if they appear invalid at the time of loading, as they
may become active after the lsm policy load. In other
words, now the lsm and the ima can be initialized in any
order and the handling logic is the same as with the lsm
rule reload event.

Patch also fixes the rule re-use during the lsm policy
reload and makes some prints a bit more human readable.

Cc: Casey Schaufler <[e-mail]>
Reported-by: Mimi Zohar <[e-mail]>
Signed-off-by: Janne Karhunen <[e-mail]>
Signed-off-by: Konsta Karsisto <[e-mail]>
---
 security/integrity/ima/ima_policy.c | 44 ++++++++++++++---------------
 1 file changed, 21 insertions(+), 23 deletions(-)

Comments

Mimi Zohar Jan. 9, 2020, 2:54 p.m. UTC | #1

On Thu, 2020-01-09 at 16:08 +0200, Janne Karhunen wrote:
> Keep the ima policy rules around from the beginning even
> if they appear invalid at the time of loading, as they
> may become active after the lsm policy load. In other
> words, now the lsm and the ima can be initialized in any
> order and the handling logic is the same as with the lsm
> rule reload event.
> 
> Patch also fixes the rule re-use during the lsm policy
> reload and makes some prints a bit more human readable.

Thanks, Janne.  What do you think about adding a single sentence at
the end of this patch description?  Something along the lines of,
"With these changes, there no need to defer loading a custom IMA
policy, based on LSM rules, until after the LSM policy has been
initialized."

The line length, here, is a bit short.  According to section "14) the
canonical path format" of Documentation/process/submitting-
patches.rst, the body of the explanation shouldl be line wrapped at 75
columns.

> 
> Cc: Casey Schaufler <[e-mail]>
> Reported-by: Mimi Zohar <[e-mail]>
> Signed-off-by: Janne Karhunen <[e-mail]>
> Signed-off-by: Konsta Karsisto <[e-mail]>

Please include a "Fixes" tag as well.  Otherwise,

Reviewed-by: Mimi Zohar <[e-mail]>

Patch

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a4dde9d575b2..4022c7736fc3 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -265,7 +265,7 @@  static void ima_lsm_free_rule(struct ima_rule_entry *entry)
 static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 {
 	struct ima_rule_entry *nentry;
-	int i, result;
+	int i;
 
 	nentry = kmalloc(sizeof(*nentry), GFP_KERNEL);
 	if (!nentry)
@@ -279,7 +279,7 @@  static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 	memset(nentry->lsm, 0, FIELD_SIZEOF(struct ima_rule_entry, lsm));
 
 	for (i = 0; i < MAX_LSM_RULES; i++) {
-		if (!entry->lsm[i].rule)
+		if (!entry->lsm[i].args_p)
 			continue;
 
 		nentry->lsm[i].type = entry->lsm[i].type;
@@ -288,13 +288,13 @@  static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 		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 undefined\n",
-				entry->lsm[i].type);
+		security_filter_rule_init(nentry->lsm[i].type,
+					  Audit_equal,
+					  nentry->lsm[i].args_p,
+					  &nentry->lsm[i].rule);
+		if (!nentry->lsm[i].rule)
+			pr_warn("rule for LSM \'%s\' is undefined\n",
+				(char *)entry->lsm[i].args_p);
 	}
 	return nentry;
 
@@ -331,7 +331,9 @@  static void ima_lsm_update_rules(void)
 	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) {
+			if (entry->lsm[i].args_p) {
+				pr_info("rule for LSM \'%s\' needs update\n",
+					(char *)entry->lsm[i].args_p);
 				needs_update = 1;
 				break;
 			}
@@ -341,8 +343,7 @@  static void ima_lsm_update_rules(void)
 
 		result = ima_lsm_update_rule(entry);
 		if (result) {
-			pr_err("ima: lsm rule update error %d\n",
-				result);
+			pr_err("lsm rule update error %d\n", result);
 			return;
 		}
 	}
@@ -865,8 +866,6 @@  static const match_table_t policy_tokens = {
 static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 			     substring_t *args, int lsm_rule, int audit_type)
 {
-	int result;
-
 	if (entry->lsm[lsm_rule].rule)
 		return -EINVAL;
 
@@ -875,16 +874,15 @@  static int ima_lsm_rule_init(struct ima_rule_entry *entry,
 		return -ENOMEM;
 
 	entry->lsm[lsm_rule].type = audit_type;
-	result = security_filter_rule_init(entry->lsm[lsm_rule].type,
-					   Audit_equal,
-					   entry->lsm[lsm_rule].args_p,
-					   &entry->lsm[lsm_rule].rule);
-	if (!entry->lsm[lsm_rule].rule) {
-		kfree(entry->lsm[lsm_rule].args_p);
-		return -EINVAL;
-	}
+	security_filter_rule_init(entry->lsm[lsm_rule].type,
+				  Audit_equal,
+				  entry->lsm[lsm_rule].args_p,
+				  &entry->lsm[lsm_rule].rule);
+	if (!entry->lsm[lsm_rule].rule)
+		pr_warn("rule for LSM \'%s\' is undefined\n",
+			(char *)entry->lsm[lsm_rule].args_p);
 
-	return result;
+	return 0;
 }
 
 static void ima_log_string_op(struct audit_buffer *ab, char *key, char *value,