Re: [PATCH] ima: fix infinite loop within "ima_match_policy" function.
flat view
From: THOBY Simon <hidden>
Date: 2021-08-19 12:58:14
Also in:
linux-integrity, lkml
Hi Liqiong, On 8/19/21 12:15 PM, liqiong wrote:
When "ima_match_policy" is looping while "ima_update_policy" changs the variable "ima_rules", then "ima_match_policy" may can't exit loop, and kernel keeps printf "rcu_sched detected stall on CPU ...". It occurs at boot phase, systemd-services are being checked within "ima_match_policy,at the same time, the variable "ima_rules" is changed by a service.
First off, thanks for finding and identifying this nasty bug.
quoted hunk ↗ jump to hunk
Signed-off-by: liqiong <redacted> --- security/integrity/ima/ima_policy.c | 5 +++++ 1 file changed, 5 insertions(+)diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index fd5d46e511f1..7e71e643457c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c@@ -217,6 +217,7 @@ static LIST_HEAD(ima_default_rules); static LIST_HEAD(ima_policy_rules); static LIST_HEAD(ima_temp_rules); static struct list_head *ima_rules = &ima_default_rules; +static DECLARE_RWSEM(ima_rules_sem); static int ima_policy __initdata;@@ -666,6 +667,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, if (template_desc && !*template_desc) *template_desc = ima_template_desc_current(); + down_read(&ima_rules_sem); rcu_read_lock(); list_for_each_entry_rcu(entry, ima_rules, list) {@@ -702,6 +704,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, break; } rcu_read_unlock(); + up_read(&ima_rules_sem); return action; }@@ -919,7 +922,9 @@ void ima_update_policy(void) if (ima_rules != policy) { ima_policy_flag = 0; + down_write(&ima_rules_sem); ima_rules = policy; + up_write(&ima_rules_sem); /* * IMA architecture specific policy rules are specified
Rather than introducing a new semaphore, I wonder if you couldn't have done something like the following?
@@ -674,13 +674,15 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode, const char *func_data, unsigned int *allowed_algos) { struct ima_rule_entry *entry; + struct list_head *ima_rules_tmp; int action = 0, actmask = flags | (flags << 1); if (template_desc && !*template_desc) *template_desc = ima_template_desc_current(); rcu_read_lock(); - list_for_each_entry_rcu(entry, ima_rules, list) { + ima_rules_tmp = rcu_dereference(ima_rules); + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { if (!(entry->action & actmask)) continue;
@@ -970,7 +972,7 @@ void ima_update_policy(void) if (ima_rules != policy) { ima_policy_flag = 0; - ima_rules = policy; + rcu_assign_pointer(ima_rules, policy); /* * IMA architecture specific policy rules are specified
Also, ima_match_policy is not the only place where we iterate over ima_rules, maybe this change should be applied to every function that perform a call the like of "list_for_each_entry_rcu(entry, ima_rules_tmp, list)" ? All that being said, your change is quite small and I have no objection to it, I was just wondering whether we could achieve the same effect without locks with RCU. What do you think? Thanks, Simon