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

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

[GUO Zihua]

IMA relies on lsm policy update notifier to be notified when it should
update it's lsm rules.

When SELinux update it's 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
be measured by IMA.

Fix it by retrying for at most three times if -ESTALE is returned by
ima_filter_rule_match().

Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: GUO Zihua <guozihua@huawei.com>
---
 security/integrity/ima/ima_policy.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a8802b8da946..6edc8cc48dec 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -609,6 +609,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
 	for (i = 0; i < MAX_LSM_RULES; i++) {
 		int rc = 0;
 		u32 osid;
+		int retried = 0;
 
 		if (!rule->lsm[i].rule) {
 			if (!rule->lsm[i].args_p)
@@ -616,6 +617,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
 			else
 				return false;
 		}
+
+retry:
 		switch (i) {
 		case LSM_OBJ_USER:
 		case LSM_OBJ_ROLE:
@@ -635,6 +638,11 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
 		default:
 			break;
 		}
+
+		if (rc == -ESTALE && retried < 3) {
+			retried++;
+			goto retry;
+		}
 		if (!rc)
 			return false;
 	}
-- 
2.17.1

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

[Mimi Zohar]

Hi Scott,

On Thu, 2022-08-18 at 10:05 +0800, GUO Zihua wrote:
> IMA relies on lsm policy update notifier to be notified when it should
> update it's lsm rules.

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

> When SELinux update it's 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
> be measured by IMA.
> 
> Fix it by retrying for at most three times if -ESTALE is returned by
> ima_filter_rule_match().

With the lazy LSM policy update, retrying only once was needed.  With
the blocking LSM notifier callback, why is three times needed?  Is this
really a function of how long it takes IMA to walk and update ALL the
LSM based IMA policy rules?  Would having SELinux wait for the -ESTALE
to change do anything?

> 
> Fixes: b16942455193 ("ima: use the lsm policy update notifier")
> Signed-off-by: GUO Zihua <guozihua@huawei.com>

thanks,

Mimi

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

[Guozihua (Scott)]

On 2022/8/18 21:43, Mimi Zohar wrote:
> Hi Scott,
> 
> On Thu, 2022-08-18 at 10:05 +0800, GUO Zihua wrote:
>> IMA relies on lsm policy update notifier to be notified when it should
>> update it's lsm rules.
> 
> ^IMA relies on the blocking LSM policy notifier callback to update the
> LSM based IMA policy rules.

I'll fix this in the next version.
> 
>> When SELinux update it's 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
>> be measured by IMA.
>>
>> Fix it by retrying for at most three times if -ESTALE is returned by
>> ima_filter_rule_match().
> 
> With the lazy LSM policy update, retrying only once was needed.  With
> the blocking LSM notifier callback, why is three times needed?  Is this
> really a function of how long it takes IMA to walk and update ALL the
> LSM based IMA policy rules?  Would having SELinux wait for the -ESTALE
> to change do anything?

With lazy policy update, policy update is triggered and would be 
finished before retrying. However, with a notifier callback, the update 
runs in a different process which might introduce extra latency. 
Technically if one rule has been updated, any following rules would have 
been updated at the time they are read as well, thus the retry should 
happen on the first rule affected by SELinux policy update only. 
Retrying for three times here would leave some time for the notifier to 
finish it's job on updating the rules.
>>
>> Fixes: b16942455193 ("ima: use the lsm policy update notifier")
>> Signed-off-by: GUO Zihua <guozihua@huawei.com>
> 
> thanks,
> 
> Mimi
> 
> .


-- 
Best
GUO Zihua

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

[Mimi Zohar]

On Fri, 2022-08-19 at 09:50 +0800, Guozihua (Scott) wrote:
> On 2022/8/18 21:43, Mimi Zohar wrote:
> > Hi Scott,
> > 
> > On Thu, 2022-08-18 at 10:05 +0800, GUO Zihua wrote:
> >> IMA relies on lsm policy update notifier to be notified when it should
> >> update it's lsm rules.
> > 
> > ^IMA relies on the blocking LSM policy notifier callback to update the
> > LSM based IMA policy rules.
> 
> I'll fix this in the next version.

Thanks.

> > 
> >> When SELinux update it's 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
> >> be measured by IMA.
> >>
> >> Fix it by retrying for at most three times if -ESTALE is returned by
> >> ima_filter_rule_match().
> > 
> > With the lazy LSM policy update, retrying only once was needed.  With
> > the blocking LSM notifier callback, why is three times needed?  Is this
> > really a function of how long it takes IMA to walk and update ALL the
> > LSM based IMA policy rules?  Would having SELinux wait for the -ESTALE
> > to change do anything?
> 
> With lazy policy update, policy update is triggered and would be 
> finished before retrying. However, with a notifier callback, the update 
> runs in a different process which might introduce extra latency. 
> Technically if one rule has been updated, any following rules would have 
> been updated at the time they are read as well, thus the retry should 
> happen on the first rule affected by SELinux policy update only. 
> Retrying for three times here would leave some time for the notifier to 
> finish it's job on updating the rules.

The question is whether we're waiting for the SELinux policy to change
from ESTALE or whether it is the number of SELinux based IMA policy
rules or some combination of the two.  Retrying three times seems to be
random.  If SELinux waited for ESTALE to change, then it would only be
dependent on the time it took to update the SELinux based IMA policy
rules.

thanks,

Mimi

> >>
> >> Fixes: b16942455193 ("ima: use the lsm policy update notifier")
> >> Signed-off-by: GUO Zihua <guozihua@huawei.com>

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

[Guozihua (Scott)]

On 2022/8/22 22:41, Mimi Zohar wrote:
> On Fri, 2022-08-19 at 09:50 +0800, Guozihua (Scott) wrote:
>> On 2022/8/18 21:43, Mimi Zohar wrote:
>>> Hi Scott,
>>>
>>> On Thu, 2022-08-18 at 10:05 +0800, GUO Zihua wrote:
>>>> IMA relies on lsm policy update notifier to be notified when it should
>>>> update it's lsm rules.
>>>
>>> ^IMA relies on the blocking LSM policy notifier callback to update the
>>> LSM based IMA policy rules.
>>
>> I'll fix this in the next version.
> 
> Thanks.
> 
>>>
>>>> When SELinux update it's 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
>>>> be measured by IMA.
>>>>
>>>> Fix it by retrying for at most three times if -ESTALE is returned by
>>>> ima_filter_rule_match().
>>>
>>> With the lazy LSM policy update, retrying only once was needed.  With
>>> the blocking LSM notifier callback, why is three times needed?  Is this
>>> really a function of how long it takes IMA to walk and update ALL the
>>> LSM based IMA policy rules?  Would having SELinux wait for the -ESTALE
>>> to change do anything?
>>
>> With lazy policy update, policy update is triggered and would be
>> finished before retrying. However, with a notifier callback, the update
>> runs in a different process which might introduce extra latency.
>> Technically if one rule has been updated, any following rules would have
>> been updated at the time they are read as well, thus the retry should
>> happen on the first rule affected by SELinux policy update only.
>> Retrying for three times here would leave some time for the notifier to
>> finish it's job on updating the rules.
> 
> The question is whether we're waiting for the SELinux policy to change
> from ESTALE or whether it is the number of SELinux based IMA policy
> rules or some combination of the two.  Retrying three times seems to be
> random.  If SELinux waited for ESTALE to change, then it would only be
> dependent on the time it took to update the SELinux based IMA policy
> rules.

We are waiting for ima_lsm_update_rules() to finish re-initializing all 
the LSM based rules.

Once new policy takes effect in SELinux, the policy sequence number 
would be incremented. During rule match, this sequence number is checked 
and if mismatched, -ESTALE is returned and the rules should be 
re-initialized. Normally during this time, ima_lsm_update_rules should 
be running already, so we are going to wait for it to finish.
> 
> thanks,
> 
> Mimi
> 
>>>>
>>>> Fixes: b16942455193 ("ima: use the lsm policy update notifier")
>>>> Signed-off-by: GUO Zihua <guozihua@huawei.com>
> 
> .


-- 
Best
GUO Zihua

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

[Mimi Zohar]

On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
> > The question is whether we're waiting for the SELinux policy to change
> > from ESTALE or whether it is the number of SELinux based IMA policy
> > rules or some combination of the two.  Retrying three times seems to be
> > random.  If SELinux waited for ESTALE to change, then it would only be
> > dependent on the time it took to update the SELinux based IMA policy
> > rules.
> 
> We are waiting for ima_lsm_update_rules() to finish re-initializing all 
> the LSM based rules.

Fine.  Hopefully retrying a maximum of 3 times is sufficient.

-- 
thanks,

Mimi

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

[Guozihua (Scott)]

On 2022/8/23 21:21, Mimi Zohar wrote:
> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
>>> The question is whether we're waiting for the SELinux policy to change
>>> from ESTALE or whether it is the number of SELinux based IMA policy
>>> rules or some combination of the two.  Retrying three times seems to be
>>> random.  If SELinux waited for ESTALE to change, then it would only be
>>> dependent on the time it took to update the SELinux based IMA policy
>>> rules.
>>
>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
>> the LSM based rules.
> 
> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
> 
Well, at least this should greatly reduce the chance of this issue from 
happening. This would be the best we I can think of without locking and 
busy waiting. Maybe we can also add delays before we retry. Maybe you 
got any other thought in mind?

-- 
Best
GUO Zihua

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

[Mimi Zohar]

On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
> On 2022/8/23 21:21, Mimi Zohar wrote:
> > On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
> >>> The question is whether we're waiting for the SELinux policy to change
> >>> from ESTALE or whether it is the number of SELinux based IMA policy
> >>> rules or some combination of the two.  Retrying three times seems to be
> >>> random.  If SELinux waited for ESTALE to change, then it would only be
> >>> dependent on the time it took to update the SELinux based IMA policy
> >>> rules.
> >>
> >> We are waiting for ima_lsm_update_rules() to finish re-initializing all
> >> the LSM based rules.
> > 
> > Fine.  Hopefully retrying a maximum of 3 times is sufficient.
> > 
> Well, at least this should greatly reduce the chance of this issue from 
> happening.

Agreed

> This would be the best we I can think of without locking and 
> busy waiting. Maybe we can also add delays before we retry. Maybe you 
> got any other thought in mind?

Another option would be to re-introduce the equivalent of the "lazy"
LSM update on -ESTALE, but without updating the policy rule, as the
notifier callback will eventually get to it.

-- 
thanks,

Mimi

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

[Guozihua (Scott)]

On 2022/8/24 9:26, Mimi Zohar wrote:
> On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
>> On 2022/8/23 21:21, Mimi Zohar wrote:
>>> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
>>>>> The question is whether we're waiting for the SELinux policy to change
>>>>> from ESTALE or whether it is the number of SELinux based IMA policy
>>>>> rules or some combination of the two.  Retrying three times seems to be
>>>>> random.  If SELinux waited for ESTALE to change, then it would only be
>>>>> dependent on the time it took to update the SELinux based IMA policy
>>>>> rules.
>>>>
>>>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
>>>> the LSM based rules.
>>>
>>> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
>>>
>> Well, at least this should greatly reduce the chance of this issue from
>> happening.
> 
> Agreed
> 
>> This would be the best we I can think of without locking and
>> busy waiting. Maybe we can also add delays before we retry. Maybe you
>> got any other thought in mind?
> 
> Another option would be to re-introduce the equivalent of the "lazy"
> LSM update on -ESTALE, but without updating the policy rule, as the
> notifier callback will eventually get to it.
> 

For this to happen we would need a way to tell when we are able to 
continue with the retry though.

-- 
Best
GUO Zihua

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

[Mimi Zohar]

On Wed, 2022-08-24 at 09:56 +0800, Guozihua (Scott) wrote:
> On 2022/8/24 9:26, Mimi Zohar wrote:
> > On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
> >> On 2022/8/23 21:21, Mimi Zohar wrote:
> >>> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
> >>>>> The question is whether we're waiting for the SELinux policy to change
> >>>>> from ESTALE or whether it is the number of SELinux based IMA policy
> >>>>> rules or some combination of the two.  Retrying three times seems to be
> >>>>> random.  If SELinux waited for ESTALE to change, then it would only be
> >>>>> dependent on the time it took to update the SELinux based IMA policy
> >>>>> rules.
> >>>>
> >>>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
> >>>> the LSM based rules.
> >>>
> >>> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
> >>>
> >> Well, at least this should greatly reduce the chance of this issue from
> >> happening.
> > 
> > Agreed
> > 
> >> This would be the best we I can think of without locking and
> >> busy waiting. Maybe we can also add delays before we retry. Maybe you
> >> got any other thought in mind?
> > 
> > Another option would be to re-introduce the equivalent of the "lazy"
> > LSM update on -ESTALE, but without updating the policy rule, as the
> > notifier callback will eventually get to it.
> > 
> 
> For this to happen we would need a way to tell when we are able to 
> continue with the retry though.

Previously with the lazy update, on failure security_filter_rule_init()
was called before the retry.  To avoid locking or detecting when to
continue, another option would be to call to
security_filter_rule_init() with a local copy of the rule.  The retry
would be based on a local copy of the rule.

Eventually the registered callback will complete, so we don't need to
be concerned about updating the actual rules.
-- 
thanks,

Mimi

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

[Guozihua (Scott)]

On 2022/8/25 21:02, Mimi Zohar wrote:
> On Wed, 2022-08-24 at 09:56 +0800, Guozihua (Scott) wrote:
>> On 2022/8/24 9:26, Mimi Zohar wrote:
>>> On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
>>>> On 2022/8/23 21:21, Mimi Zohar wrote:
>>>>> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
>>>>>>> The question is whether we're waiting for the SELinux policy to change
>>>>>>> from ESTALE or whether it is the number of SELinux based IMA policy
>>>>>>> rules or some combination of the two.  Retrying three times seems to be
>>>>>>> random.  If SELinux waited for ESTALE to change, then it would only be
>>>>>>> dependent on the time it took to update the SELinux based IMA policy
>>>>>>> rules.
>>>>>>
>>>>>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
>>>>>> the LSM based rules.
>>>>>
>>>>> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
>>>>>
>>>> Well, at least this should greatly reduce the chance of this issue from
>>>> happening.
>>>
>>> Agreed
>>>
>>>> This would be the best we I can think of without locking and
>>>> busy waiting. Maybe we can also add delays before we retry. Maybe you
>>>> got any other thought in mind?
>>>
>>> Another option would be to re-introduce the equivalent of the "lazy"
>>> LSM update on -ESTALE, but without updating the policy rule, as the
>>> notifier callback will eventually get to it.
>>>
>>
>> For this to happen we would need a way to tell when we are able to
>> continue with the retry though.
> 
> Previously with the lazy update, on failure security_filter_rule_init()
> was called before the retry.  To avoid locking or detecting when to
> continue, another option would be to call to
> security_filter_rule_init() with a local copy of the rule.  The retry
> would be based on a local copy of the rule.
> 
> Eventually the registered callback will complete, so we don't need to
> be concerned about updating the actual rules.

Is it possible to cause race condition though? With this, the notifier 
path seems to be unnecessary.

-- 
Best
GUO Zihua

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

[Mimi Zohar]

On Sat, 2022-08-27 at 17:57 +0800, Guozihua (Scott) wrote:
> On 2022/8/25 21:02, Mimi Zohar wrote:
> > On Wed, 2022-08-24 at 09:56 +0800, Guozihua (Scott) wrote:
> >> On 2022/8/24 9:26, Mimi Zohar wrote:
> >>> On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
> >>>> On 2022/8/23 21:21, Mimi Zohar wrote:
> >>>>> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
> >>>>>>> The question is whether we're waiting for the SELinux policy to change
> >>>>>>> from ESTALE or whether it is the number of SELinux based IMA policy
> >>>>>>> rules or some combination of the two.  Retrying three times seems to be
> >>>>>>> random.  If SELinux waited for ESTALE to change, then it would only be
> >>>>>>> dependent on the time it took to update the SELinux based IMA policy
> >>>>>>> rules.
> >>>>>>
> >>>>>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
> >>>>>> the LSM based rules.
> >>>>>
> >>>>> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
> >>>>>
> >>>> Well, at least this should greatly reduce the chance of this issue from
> >>>> happening.
> >>>
> >>> Agreed
> >>>
> >>>> This would be the best we I can think of without locking and
> >>>> busy waiting. Maybe we can also add delays before we retry. Maybe you
> >>>> got any other thought in mind?
> >>>
> >>> Another option would be to re-introduce the equivalent of the "lazy"
> >>> LSM update on -ESTALE, but without updating the policy rule, as the
> >>> notifier callback will eventually get to it.
> >>>
> >>
> >> For this to happen we would need a way to tell when we are able to
> >> continue with the retry though.
> > 
> > Previously with the lazy update, on failure security_filter_rule_init()
> > was called before the retry.  To avoid locking or detecting when to
> > continue, another option would be to call to
> > security_filter_rule_init() with a local copy of the rule.  The retry
> > would be based on a local copy of the rule.
> > 
> > Eventually the registered callback will complete, so we don't need to
> > be concerned about updating the actual rules.
> 
> Is it possible to cause race condition though? With this, the notifier 
> path seems to be unnecessary.

I don't see how there would be a race condition.  The notifier callback
is the normal method of updating the policy rules.  Hopefully -ESTALE
isn't something that happens frequently.
-- 
thanks,

Mimi

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

[Guozihua (Scott)]

On 2022/8/30 9:20, Mimi Zohar wrote:
> On Sat, 2022-08-27 at 17:57 +0800, Guozihua (Scott) wrote:
>> On 2022/8/25 21:02, Mimi Zohar wrote:
>>> On Wed, 2022-08-24 at 09:56 +0800, Guozihua (Scott) wrote:
>>>> On 2022/8/24 9:26, Mimi Zohar wrote:
>>>>> On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
>>>>>> On 2022/8/23 21:21, Mimi Zohar wrote:
>>>>>>> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
>>>>>>>>> The question is whether we're waiting for the SELinux policy to change
>>>>>>>>> from ESTALE or whether it is the number of SELinux based IMA policy
>>>>>>>>> rules or some combination of the two.  Retrying three times seems to be
>>>>>>>>> random.  If SELinux waited for ESTALE to change, then it would only be
>>>>>>>>> dependent on the time it took to update the SELinux based IMA policy
>>>>>>>>> rules.
>>>>>>>>
>>>>>>>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
>>>>>>>> the LSM based rules.
>>>>>>>
>>>>>>> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
>>>>>>>
>>>>>> Well, at least this should greatly reduce the chance of this issue from
>>>>>> happening.
>>>>>
>>>>> Agreed
>>>>>
>>>>>> This would be the best we I can think of without locking and
>>>>>> busy waiting. Maybe we can also add delays before we retry. Maybe you
>>>>>> got any other thought in mind?
>>>>>
>>>>> Another option would be to re-introduce the equivalent of the "lazy"
>>>>> LSM update on -ESTALE, but without updating the policy rule, as the
>>>>> notifier callback will eventually get to it.
>>>>>
>>>>
>>>> For this to happen we would need a way to tell when we are able to
>>>> continue with the retry though.
>>>
>>> Previously with the lazy update, on failure security_filter_rule_init()
>>> was called before the retry.  To avoid locking or detecting when to
>>> continue, another option would be to call to
>>> security_filter_rule_init() with a local copy of the rule.  The retry
>>> would be based on a local copy of the rule.
>>>
>>> Eventually the registered callback will complete, so we don't need to
>>> be concerned about updating the actual rules.
>>
>> Is it possible to cause race condition though? With this, the notifier
>> path seems to be unnecessary.
> 
> I don't see how there would be a race condition.  The notifier callback
> is the normal method of updating the policy rules.  Hopefully -ESTALE
> isn't something that happens frequently.

The notifier callback uses RCU to update rules, I think we should mimic 
that behavior if we are to update individual rules in the matching logic.

-- 
Best
GUO Zihua

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

[Mimi Zohar]

On Tue, 2022-08-30 at 16:41 +0800, Guozihua (Scott) wrote:
> On 2022/8/30 9:20, Mimi Zohar wrote:
> > On Sat, 2022-08-27 at 17:57 +0800, Guozihua (Scott) wrote:
> >> On 2022/8/25 21:02, Mimi Zohar wrote:
> >>> On Wed, 2022-08-24 at 09:56 +0800, Guozihua (Scott) wrote:
> >>>> On 2022/8/24 9:26, Mimi Zohar wrote:
> >>>>> On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
> >>>>>> On 2022/8/23 21:21, Mimi Zohar wrote:
> >>>>>>> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
> >>>>>>>>> The question is whether we're waiting for the SELinux policy to change
> >>>>>>>>> from ESTALE or whether it is the number of SELinux based IMA policy
> >>>>>>>>> rules or some combination of the two.  Retrying three times seems to be
> >>>>>>>>> random.  If SELinux waited for ESTALE to change, then it would only be
> >>>>>>>>> dependent on the time it took to update the SELinux based IMA policy
> >>>>>>>>> rules.
> >>>>>>>>
> >>>>>>>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
> >>>>>>>> the LSM based rules.
> >>>>>>>
> >>>>>>> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
> >>>>>>>
> >>>>>> Well, at least this should greatly reduce the chance of this issue from
> >>>>>> happening.
> >>>>>
> >>>>> Agreed
> >>>>>
> >>>>>> This would be the best we I can think of without locking and
> >>>>>> busy waiting. Maybe we can also add delays before we retry. Maybe you
> >>>>>> got any other thought in mind?
> >>>>>
> >>>>> Another option would be to re-introduce the equivalent of the "lazy"
> >>>>> LSM update on -ESTALE, but without updating the policy rule, as the
> >>>>> notifier callback will eventually get to it.
> >>>>>
> >>>>
> >>>> For this to happen we would need a way to tell when we are able to
> >>>> continue with the retry though.
> >>>
> >>> Previously with the lazy update, on failure security_filter_rule_init()
> >>> was called before the retry.  To avoid locking or detecting when to
> >>> continue, another option would be to call to
> >>> security_filter_rule_init() with a local copy of the rule.  The retry
> >>> would be based on a local copy of the rule.
> >>>
> >>> Eventually the registered callback will complete, so we don't need to
> >>> be concerned about updating the actual rules.
> >>
> >> Is it possible to cause race condition though? With this, the notifier
> >> path seems to be unnecessary.
> > 
> > I don't see how there would be a race condition.  The notifier callback
> > is the normal method of updating the policy rules.  Hopefully -ESTALE
> > isn't something that happens frequently.
> 
> The notifier callback uses RCU to update rules, I think we should mimic 
> that behavior if we are to update individual rules in the matching logic.

If the callback update hasn't completed causing an -ESTALE, the
fallback is to directly query the LSM for a single IMA policy rule. 
Please keep it simple.

-- 
thanks,

Mimi

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

[Guozihua (Scott)]

On 2022/8/30 20:03, Mimi Zohar wrote:
> On Tue, 2022-08-30 at 16:41 +0800, Guozihua (Scott) wrote:
>> On 2022/8/30 9:20, Mimi Zohar wrote:
>>> On Sat, 2022-08-27 at 17:57 +0800, Guozihua (Scott) wrote:
>>>> On 2022/8/25 21:02, Mimi Zohar wrote:
>>>>> On Wed, 2022-08-24 at 09:56 +0800, Guozihua (Scott) wrote:
>>>>>> On 2022/8/24 9:26, Mimi Zohar wrote:
>>>>>>> On Tue, 2022-08-23 at 21:28 +0800, Guozihua (Scott) wrote:
>>>>>>>> On 2022/8/23 21:21, Mimi Zohar wrote:
>>>>>>>>> On Tue, 2022-08-23 at 16:12 +0800, Guozihua (Scott) wrote:
>>>>>>>>>>> The question is whether we're waiting for the SELinux policy to change
>>>>>>>>>>> from ESTALE or whether it is the number of SELinux based IMA policy
>>>>>>>>>>> rules or some combination of the two.  Retrying three times seems to be
>>>>>>>>>>> random.  If SELinux waited for ESTALE to change, then it would only be
>>>>>>>>>>> dependent on the time it took to update the SELinux based IMA policy
>>>>>>>>>>> rules.
>>>>>>>>>>
>>>>>>>>>> We are waiting for ima_lsm_update_rules() to finish re-initializing all
>>>>>>>>>> the LSM based rules.
>>>>>>>>>
>>>>>>>>> Fine.  Hopefully retrying a maximum of 3 times is sufficient.
>>>>>>>>>
>>>>>>>> Well, at least this should greatly reduce the chance of this issue from
>>>>>>>> happening.
>>>>>>>
>>>>>>> Agreed
>>>>>>>
>>>>>>>> This would be the best we I can think of without locking and
>>>>>>>> busy waiting. Maybe we can also add delays before we retry. Maybe you
>>>>>>>> got any other thought in mind?
>>>>>>>
>>>>>>> Another option would be to re-introduce the equivalent of the "lazy"
>>>>>>> LSM update on -ESTALE, but without updating the policy rule, as the
>>>>>>> notifier callback will eventually get to it.
>>>>>>>
>>>>>>
>>>>>> For this to happen we would need a way to tell when we are able to
>>>>>> continue with the retry though.
>>>>>
>>>>> Previously with the lazy update, on failure security_filter_rule_init()
>>>>> was called before the retry.  To avoid locking or detecting when to
>>>>> continue, another option would be to call to
>>>>> security_filter_rule_init() with a local copy of the rule.  The retry
>>>>> would be based on a local copy of the rule.
>>>>>
>>>>> Eventually the registered callback will complete, so we don't need to
>>>>> be concerned about updating the actual rules.
>>>>
>>>> Is it possible to cause race condition though? With this, the notifier
>>>> path seems to be unnecessary.
>>>
>>> I don't see how there would be a race condition.  The notifier callback
>>> is the normal method of updating the policy rules.  Hopefully -ESTALE
>>> isn't something that happens frequently.
>>
>> The notifier callback uses RCU to update rules, I think we should mimic
>> that behavior if we are to update individual rules in the matching logic.
> 
> If the callback update hasn't completed causing an -ESTALE, the
> fallback is to directly query the LSM for a single IMA policy rule.
> Please keep it simple.
> 

Got it, I'll send a new patch.

-- 
Best
GUO Zihua