[PATCH 0/2] ima: Fix IMA mishandling of LSM based rule during

[PATCH 0/2] ima: Fix IMA mishandling of LSM based rule during

[GUO Zihua]

Backports the following two patches to fix the issue of IMA mishandling
LSM based rule during LSM policy update, causing a file to match an
unexpected rule.

Some changes were made to these patches, which was stated in the commit
message of corresponding patch.

GUO Zihua (1):
  ima: Handle -ESTALE returned by ima_filter_rule_match()

Janne Karhunen (1):
  ima: use the lsm policy update notifier

 security/integrity/ima/ima.h        |   2 +
 security/integrity/ima/ima_main.c   |   8 ++
 security/integrity/ima/ima_policy.c | 146 +++++++++++++++++++++++-----
 3 files changed, 130 insertions(+), 26 deletions(-)

-- 
2.17.1

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

[GUO Zihua]

From: Janne Karhunen <janne.karhunen@gmail.com>

[ Upstream commit b169424551930a9325f700f502802f4d515194e5 ]

This patch is backported to resolve the issue of IMA ignoreing LSM part of
an LSM based rule. As the LSM notifier chain was an atomic notifier
chain, we'll not be able to call synchronize_rcu() within our notifier
handling function.

Original patch message is as follows:

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

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

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: stable@vger.kernel.org #4.19.y
Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 security/integrity/ima/ima.h        |   2 +
 security/integrity/ima/ima_main.c   |   8 ++
 security/integrity/ima/ima_policy.c | 115 +++++++++++++++++++++++-----
 3 files changed, 105 insertions(+), 20 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e2916b115b93..dc564ed9a790 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ int ima_measurements_show(struct seq_file *m, void *v);
 unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(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 2d31921fbda4..f461b3e2de00 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -41,6 +41,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();
@@ -553,6 +557,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 b2dadff3626b..1e0251e9510a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -256,31 +256,112 @@ static void ima_free_rule(struct ima_rule_entry *entry)
 	kfree(entry);
 }
 
+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);
+}
+
+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;
+
+	/*
+	 * Immutable elements are copied over as pointers and data; only
+	 * lsm rules can change
+	 */
+	memcpy(nentry, entry, sizeof(*nentry));
+	memset(nentry->lsm, 0, FIELD_SIZEOF(struct ima_rule_entry, lsm));
+
+	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 undefined\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);
+	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
@@ -334,11 +415,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:
@@ -361,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

[PATCH 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[GUO Zihua]

[ Upstream commit c7423dbdbc9ecef7fff5239d144cad4b9887f4de ]

IMA relies on the blocking LSM policy notifier callback to update the
LSM based IMA policy rules.

When SELinux update its policies, IMA would be notified and starts
updating all its lsm rules one-by-one. During this time, -ESTALE would
be returned by ima_filter_rule_match() if it is called with a LSM rule
that has not yet been updated. In ima_match_rules(), -ESTALE is not
handled, and the LSM rule is considered a match, causing extra files
to be measured by IMA.

Fix it by re-initializing a temporary rule if -ESTALE is returned by
ima_filter_rule_match(). The origin rule in the rule list would be
updated by the LSM policy notifier callback.

Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: GUO Zihua <guozihua@huawei.com>
Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: stable@vger.kernel.org # 4.19.y
---
 security/integrity/ima/ima_policy.c | 34 ++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1e0251e9510a..dd146a34a53a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -378,6 +378,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			    enum ima_hooks func, int mask)
 {
 	int i;
+	bool result = false;
+	struct ima_rule_entry *lsm_rule = rule;
+	bool rule_reinitialized = false;
 
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
@@ -416,35 +419,50 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 		int rc = 0;
 		u32 osid;
 
-		if (!rule->lsm[i].rule)
+		if (!lsm_rule->lsm[i].rule)
 			continue;
 
+retry:
 		switch (i) {
 		case LSM_OBJ_USER:
 		case LSM_OBJ_ROLE:
 		case LSM_OBJ_TYPE:
 			security_inode_getsecid(inode, &osid);
 			rc = security_filter_rule_match(osid,
-							rule->lsm[i].type,
+							lsm_rule->lsm[i].type,
 							Audit_equal,
-							rule->lsm[i].rule,
+							lsm_rule->lsm[i].rule,
 							NULL);
 			break;
 		case LSM_SUBJ_USER:
 		case LSM_SUBJ_ROLE:
 		case LSM_SUBJ_TYPE:
 			rc = security_filter_rule_match(secid,
-							rule->lsm[i].type,
+							lsm_rule->lsm[i].type,
 							Audit_equal,
-							rule->lsm[i].rule,
+							lsm_rule->lsm[i].rule,
 							NULL);
 		default:
 			break;
 		}
-		if (!rc)
-			return false;
+		if (rc == -ESTALE && !rule_reinitialized) {
+			lsm_rule = ima_lsm_copy_rule(rule);
+			if (lsm_rule) {
+				rule_reinitialized = true;
+				goto retry;
+			}
+		}
+		if (!rc) {
+			result = false;
+			goto out;
+		}
 	}
-	return true;
+	result = true;
+
+out:
+	if (rule_reinitialized)
+		ima_lsm_free_rule(lsm_rule);
+	return result;
 }
 
 /*
-- 
2.17.1

Re: [PATCH 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[Greg KH]

On Tue, Dec 27, 2022 at 09:47:29AM +0800, GUO Zihua wrote:
> [ Upstream commit c7423dbdbc9ecef7fff5239d144cad4b9887f4de ]

For obvious reasons we can not only take this patch (from 6.2-rc1) into
4.19.y as that would cause people who upgrade from 4.19.y to a newer
stable kernel to have a regression.

Please submit backports for all stable kernels if you wish to see this
in older ones to prevent problems like this from happening.

But also, why are you still on 4.19.y?  What is wrong with 5.4.y at this
point in time?  If we dropped support for 4.19.y in January, what would
that cause to happen for your systems?

thanks,

greg k-h

Re: [PATCH 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[Mimi Zohar]

On Tue, 2022-12-27 at 08:37 +0100, Greg KH wrote:
> On Tue, Dec 27, 2022 at 09:47:29AM +0800, GUO Zihua wrote:
> > [ Upstream commit c7423dbdbc9ecef7fff5239d144cad4b9887f4de ]
> 
> For obvious reasons we can not only take this patch (from 6.2-rc1) into
> 4.19.y as that would cause people who upgrade from 4.19.y to a newer
> stable kernel to have a regression.
> 
> Please submit backports for all stable kernels if you wish to see this
> in older ones to prevent problems like this from happening.

Sasha has already queued the original commit and the dependencies for
the 6.1, 6.0, and 5.15 stable kernels.  Those kernels all had the
call_lsm_notifier() to call_blocking_lsm_notifier() change.  Prior to
5.3, the change to the blocking notifier would need to be backported as
well.  This version of the backport still needs to be reviewed.

thanks,

Mimi

> 
> But also, why are you still on 4.19.y?  What is wrong with 5.4.y at this
> point in time?  If we dropped support for 4.19.y in January, what would
> that cause to happen for your systems?
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH 2/2] ima: Handle -ESTALE returned by ima_filter_rule_match()

[Guozihua (Scott)]

Hi Greg and Mimi.

Fall sick for a couple days.

On 2022/12/27 19:56, Mimi Zohar wrote:
> On Tue, 2022-12-27 at 08:37 +0100, Greg KH wrote:
>> On Tue, Dec 27, 2022 at 09:47:29AM +0800, GUO Zihua wrote:
>>> [ Upstream commit c7423dbdbc9ecef7fff5239d144cad4b9887f4de ]
>>
>> For obvious reasons we can not only take this patch (from 6.2-rc1) into
>> 4.19.y as that would cause people who upgrade from 4.19.y to a newer
>> stable kernel to have a regression.
>>
>> Please submit backports for all stable kernels if you wish to see this
>> in older ones to prevent problems like this from happening.
> 
> Sasha has already queued the original commit and the dependencies for
> the 6.1, 6.0, and 5.15 stable kernels.  Those kernels all had the
> call_lsm_notifier() to call_blocking_lsm_notifier() change.  Prior to
> 5.3, the change to the blocking notifier would need to be backported as
> well.  This version of the backport still needs to be reviewed.
Indeed the current solution needs further testing and review. One of the
concern raised by Huaxin is a possible UAF caused by the call to free
rule in update_rule. Will it be possible to backport also the change
which turn call_lsm_notifier() into call_blocking_lsm_notifier()?
> 
> thanks,
> 
> Mimi
> 
>>
>> But also, why are you still on 4.19.y?  What is wrong with 5.4.y at this
>> point in time?  If we dropped support for 4.19.y in January, what would
>> that cause to happen for your systems?
Well it's all about backward compatibility. We still got some products
using the 4.19.y LTS kernel and we would still needs to provide support
for this version of the kernel. If 4.19.y got EOL or EOS in January next
year, our company surely would develop corresponding plans to handle
that change.
>>
>> thanks,
>>
>> greg k-h
>>
> 

-- 
Best
GUO Zihua