Hi, Simon:
This solution is better then rwsem, a temp "ima_rules" variable should
can fix. I also have a another idea, with a little trick, default list
can traverse to the new list, so we don't need care about the read side.
here is the patch:
@@ -918,8 +918,21 @@ void ima_update_policy(void)
list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
if (ima_rules != policy) {
+ struct list_head *prev_rules = ima_rules;
+ struct list_head *first = ima_rules->next;
ima_policy_flag = 0;
+
+ /*
+ * Make the previous list can traverse to new list,
+ * that is tricky, or there is a deadly loop whithin
+ * "list_for_each_entry_rcu(entry, ima_rules, list)"
+ *
+ * After update "ima_rules", restore the previous list.
+ */
+ prev_rules->next = policy->next;
ima_rules = policy;
+ syncchronize_rcu();
+ prev_rules->next = first;
The side effect is the "ima_default_rules" will be changed a little while.
But it make sense, the process should be checked again by the new policy.
This patch has been tested, if will do, I can resubmit this patch.
How about this ?
----------
Regards,
liqiong
在 2021年08月19日 20:58, THOBY Simon 写道:
> 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.
>
>> Signed-off-by: liqiong <liqiong@xxxxxxxxxxxx>
>> ---
>> 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
--
李力琼<liqiong@xxxxxxxxxxxx> 13524287433
上海市浦东新区海科路99号中科院上海高等研究院3号楼3楼
