* Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
@ 2022-08-08 3:02 Guozihua (Scott)
2022-08-08 3:19 ` Guozihua (Scott)
0 siblings, 1 reply; 10+ messages in thread
From: Guozihua (Scott) @ 2022-08-08 3:02 UTC (permalink / raw)
To: linux-integrity; +Cc: Mimi Zohar, xiujianfeng, luhuaxin
Hi Community,
Recently we discovered a race condition while updating SELinux policy
with IMA lsm rule enabled. Which would lead to extra files being measured.
While SELinux policy is updated, the IDs for object types and such would
be changed, and ima_lsm_update_rules would be called.
There are no lock applied in ima_lsm_update_rules. If user accesses a
file during this time, ima_match_rules will be matching rules based on
old SELinux au_seqno resulting in selinux_audit_rule_match returning
-ESTALE.
However, in ima_match_rules, this error number is not handled, causing
IMA to think the LSM rule is also a match, leading to measuring extra files.
Relevant codes are as follows:
> static void ima_lsm_update_rules(void)
> {
> struct ima_rule_entry *entry, *e;
> int result;
>
> list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
> if (!ima_rule_contains_lsm_cond(entry))
> continue;
>
> result = ima_lsm_update_rule(entry);
A RCU style update is used with no lock applied. Reading to rules would
return rules with staled au_seqno.
> if (result) {
> pr_err("lsm rule update error %d\n", result);
> return;
> }
> }
> }
> int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
> {
> struct selinux_state *state = &selinux_state;
> struct selinux_policy *policy;
> struct context *ctxt;
> struct mls_level *level;
> struct selinux_audit_rule *rule = vrule;
> int match = 0;
>
> if (unlikely(!rule)) {
> WARN_ONCE(1, "selinux_audit_rule_match: missing rule\n");
> return -ENOENT;
> }
>
> if (!selinux_initialized(state))
> return 0;
>
> rcu_read_lock();
>
> policy = rcu_dereference(state->policy);
>
> if (rule->au_seqno < policy->latest_granting) {
> match = -ESTALE;
> goto out;
> }
SELinux would return -ESTALE here.
> static bool ima_match_rules(struct ima_rule_entry *rule,
> struct user_namespace *mnt_userns,
> struct inode *inode, const struct cred *cred,
> u32 secid, enum ima_hooks func, int mask,
> const char *func_data)
> {
> ...
>
> for (i = 0; i < MAX_LSM_RULES; i++) {
> int rc = 0;
> u32 osid;
>
> if (!rule->lsm[i].rule) {
> if (!rule->lsm[i].args_p)
> continue;
> else
> return false;
> }
> switch (i) {
> case LSM_OBJ_USER:
> case LSM_OBJ_ROLE:
> case LSM_OBJ_TYPE:
> security_inode_getsecid(inode, &osid);
> rc = ima_filter_rule_match(osid, rule->lsm[i].type,
> Audit_equal,
> rule->lsm[i].rule);
rc here will be -ESTALE.
> break;
> case LSM_SUBJ_USER:
> case LSM_SUBJ_ROLE:
> case LSM_SUBJ_TYPE:
> rc = ima_filter_rule_match(secid, rule->lsm[i].type,
> Audit_equal,
> rule->lsm[i].rule);
> break;
> default:
> break;
> }
> if (!rc)
> return false;
-ERRNO is not handled, this func will return true.
> }
> return true;
> }
It seems that IMA would not "leak" any files, but widening the
measurement range.
Is this the intended behavior? Or is it a good idea to add a lock for
LSM rules during update?
--
Best
GUO Zihua
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-08 3:02 Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules Guozihua (Scott)
@ 2022-08-08 3:19 ` Guozihua (Scott)
2022-08-09 16:24 ` Paul Moore
0 siblings, 1 reply; 10+ messages in thread
From: Guozihua (Scott) @ 2022-08-08 3:19 UTC (permalink / raw)
To: linux-integrity, selinux; +Cc: Mimi Zohar, xiujianfeng, luhuaxin, paul
On 2022/8/8 11:02, Guozihua (Scott) wrote:
> Hi Community,
>
> Recently we discovered a race condition while updating SELinux policy
> with IMA lsm rule enabled. Which would lead to extra files being measured.
>
> While SELinux policy is updated, the IDs for object types and such would
> be changed, and ima_lsm_update_rules would be called.
>
> There are no lock applied in ima_lsm_update_rules. If user accesses a
> file during this time, ima_match_rules will be matching rules based on
> old SELinux au_seqno resulting in selinux_audit_rule_match returning
> -ESTALE.
>
> However, in ima_match_rules, this error number is not handled, causing
> IMA to think the LSM rule is also a match, leading to measuring extra
> files.
>
> Relevant codes are as follows:
>
>> static void ima_lsm_update_rules(void)
>> {
>> struct ima_rule_entry *entry, *e;
>> int result;
>>
>> list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
>> if (!ima_rule_contains_lsm_cond(entry))
>> continue;
>>
>> result = ima_lsm_update_rule(entry);
>
> A RCU style update is used with no lock applied. Reading to rules would
> return rules with staled au_seqno.
>
>> if (result) {
>> pr_err("lsm rule update error %d\n", result);
>> return;
>> }
>> }
>> }
>
>
>> int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
>> {
>> struct selinux_state *state = &selinux_state;
>> struct selinux_policy *policy;
>> struct context *ctxt;
>> struct mls_level *level;
>> struct selinux_audit_rule *rule = vrule;
>> int match = 0;
>>
>> if (unlikely(!rule)) {
>> WARN_ONCE(1, "selinux_audit_rule_match: missing rule\n");
>> return -ENOENT;
>> }
>>
>> if (!selinux_initialized(state))
>> return 0;
>>
>> rcu_read_lock();
>>
>> policy = rcu_dereference(state->policy);
>>
>> if (rule->au_seqno < policy->latest_granting) {
>> match = -ESTALE;
>> goto out;
>> }
>
> SELinux would return -ESTALE here.
>
>> static bool ima_match_rules(struct ima_rule_entry *rule,
>> struct user_namespace *mnt_userns,
>> struct inode *inode, const struct cred *cred,
>> u32 secid, enum ima_hooks func, int mask,
>> const char *func_data)
>> {
>> ...
>>
>> for (i = 0; i < MAX_LSM_RULES; i++) {
>> int rc = 0;
>> u32 osid;
>>
>> if (!rule->lsm[i].rule) {
>> if (!rule->lsm[i].args_p)
>> continue;
>> else
>> return false;
>> }
>> switch (i) {
>> case LSM_OBJ_USER:
>> case LSM_OBJ_ROLE:
>> case LSM_OBJ_TYPE:
>> security_inode_getsecid(inode, &osid);
>> rc = ima_filter_rule_match(osid, rule->lsm[i].type,
>> Audit_equal,
>> rule->lsm[i].rule);
>
> rc here will be -ESTALE.
>
>> break;
>> case LSM_SUBJ_USER:
>> case LSM_SUBJ_ROLE:
>> case LSM_SUBJ_TYPE:
>> rc = ima_filter_rule_match(secid, rule->lsm[i].type,
>> Audit_equal,
>> rule->lsm[i].rule);
>> break;
>> default:
>> break;
>> }
>> if (!rc)
>> return false;
>
> -ERRNO is not handled, this func will return true.
>
>> }
>> return true;
>> }
>
> It seems that IMA would not "leak" any files, but widening the
> measurement range.
>
> Is this the intended behavior? Or is it a good idea to add a lock for
> LSM rules during update?
>
Including SELinux in the loop.
--
Best
GUO Zihua
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-08 3:19 ` Guozihua (Scott)
@ 2022-08-09 16:24 ` Paul Moore
2022-08-14 18:30 ` Mimi Zohar
0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2022-08-09 16:24 UTC (permalink / raw)
To: Guozihua (Scott)
Cc: linux-integrity, selinux, Mimi Zohar, xiujianfeng, luhuaxin
On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott) <guozihua@huawei.com> wrote:
>
> On 2022/8/8 11:02, Guozihua (Scott) wrote:
> > Hi Community,
> >
> > Recently we discovered a race condition while updating SELinux policy
> > with IMA lsm rule enabled. Which would lead to extra files being measured.
> >
> > While SELinux policy is updated, the IDs for object types and such would
> > be changed, and ima_lsm_update_rules would be called.
> >
> > There are no lock applied in ima_lsm_update_rules. If user accesses a
> > file during this time, ima_match_rules will be matching rules based on
> > old SELinux au_seqno resulting in selinux_audit_rule_match returning
> > -ESTALE.
> >
> > However, in ima_match_rules, this error number is not handled, causing
> > IMA to think the LSM rule is also a match, leading to measuring extra
> > files.
...
> > Is this the intended behavior? Or is it a good idea to add a lock for
> > LSM rules during update?
I'm not the IMA expert here, but a lot of effort has been into the
SELinux code to enable lockless/RCU SELinux policy access and I
*really* don't want to have to backtrack on that.
--
paul-moore.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-09 16:24 ` Paul Moore
@ 2022-08-14 18:30 ` Mimi Zohar
2022-08-15 22:23 ` Paul Moore
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2022-08-14 18:30 UTC (permalink / raw)
To: Paul Moore, Guozihua (Scott)
Cc: linux-integrity, selinux, xiujianfeng, luhuaxin
Hi Scott, Paul,
On Tue, 2022-08-09 at 12:24 -0400, Paul Moore wrote:
> On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott) <guozihua@huawei.com> wrote:
> >
> > On 2022/8/8 11:02, Guozihua (Scott) wrote:
> > > Hi Community,
> > >
> > > Recently we discovered a race condition while updating SELinux policy
> > > with IMA lsm rule enabled. Which would lead to extra files being measured.
> > >
> > > While SELinux policy is updated, the IDs for object types and such would
> > > be changed, and ima_lsm_update_rules would be called.
> > >
> > > There are no lock applied in ima_lsm_update_rules. If user accesses a
> > > file during this time, ima_match_rules will be matching rules based on
> > > old SELinux au_seqno resulting in selinux_audit_rule_match returning
> > > -ESTALE.
> > >
> > > However, in ima_match_rules, this error number is not handled, causing
> > > IMA to think the LSM rule is also a match, leading to measuring extra
> > > files.
>
> ...
>
> > > Is this the intended behavior? Or is it a good idea to add a lock for
> > > LSM rules during update?
>
> I'm not the IMA expert here, but a lot of effort has been into the
> SELinux code to enable lockless/RCU SELinux policy access and I
> *really* don't want to have to backtrack on that.
IMA initially updated it's reference to the SELinux label ids lazily.
More recently IMA refreshes the LSM label ids based on
register_blocking_lsm_notifier(). As a result of commit 9ad6e9cb39c6
("selinux: fix race between old and new sidtab"), -ESTALE is now being
returned.
- How likely is it if one SELinux label is stale that other labels are
stale as well?
- Perhaps SELinux is calling the call_blocking_lsm_notifier() too
early. Or does SELinux need to call the notifier again after
addressing the ESTALE ids?
thanks,
Mimib
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-14 18:30 ` Mimi Zohar
@ 2022-08-15 22:23 ` Paul Moore
2022-08-17 7:17 ` Guozihua (Scott)
0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2022-08-15 22:23 UTC (permalink / raw)
To: Mimi Zohar
Cc: Guozihua (Scott), linux-integrity, selinux, xiujianfeng, luhuaxin
On Sun, Aug 14, 2022 at 2:30 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Scott, Paul,
>
> On Tue, 2022-08-09 at 12:24 -0400, Paul Moore wrote:
> > On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott) <guozihua@huawei.com> wrote:
> > >
> > > On 2022/8/8 11:02, Guozihua (Scott) wrote:
> > > > Hi Community,
> > > >
> > > > Recently we discovered a race condition while updating SELinux policy
> > > > with IMA lsm rule enabled. Which would lead to extra files being measured.
> > > >
> > > > While SELinux policy is updated, the IDs for object types and such would
> > > > be changed, and ima_lsm_update_rules would be called.
> > > >
> > > > There are no lock applied in ima_lsm_update_rules. If user accesses a
> > > > file during this time, ima_match_rules will be matching rules based on
> > > > old SELinux au_seqno resulting in selinux_audit_rule_match returning
> > > > -ESTALE.
> > > >
> > > > However, in ima_match_rules, this error number is not handled, causing
> > > > IMA to think the LSM rule is also a match, leading to measuring extra
> > > > files.
> >
> > ...
> >
> > > > Is this the intended behavior? Or is it a good idea to add a lock for
> > > > LSM rules during update?
> >
> > I'm not the IMA expert here, but a lot of effort has been into the
> > SELinux code to enable lockless/RCU SELinux policy access and I
> > *really* don't want to have to backtrack on that.
>
> IMA initially updated it's reference to the SELinux label ids lazily.
> More recently IMA refreshes the LSM label ids based on
> register_blocking_lsm_notifier(). As a result of commit 9ad6e9cb39c6
> ("selinux: fix race between old and new sidtab"), -ESTALE is now being
> returned.
To be clear, are you seeing this only started happening after commit
9ad6e9cb39c6? If that is the case, I would suggest a retry loop
around ima_filter_rule_match() when -ESTALE is returned. I believe
that should resolve the problem, if not please let us know.
--
paul-moore.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-15 22:23 ` Paul Moore
@ 2022-08-17 7:17 ` Guozihua (Scott)
2022-08-17 7:20 ` Guozihua (Scott)
0 siblings, 1 reply; 10+ messages in thread
From: Guozihua (Scott) @ 2022-08-17 7:17 UTC (permalink / raw)
To: Paul Moore, Mimi Zohar; +Cc: linux-integrity, selinux, xiujianfeng, luhuaxin
On 2022/8/16 6:23, Paul Moore wrote:
> On Sun, Aug 14, 2022 at 2:30 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> Hi Scott, Paul,
>>
>> On Tue, 2022-08-09 at 12:24 -0400, Paul Moore wrote:
>>> On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott) <guozihua@huawei.com> wrote:
>>>>
>>>> On 2022/8/8 11:02, Guozihua (Scott) wrote:
>>>>> Hi Community,
>>>>>
>>>>> Recently we discovered a race condition while updating SELinux policy
>>>>> with IMA lsm rule enabled. Which would lead to extra files being measured.
>>>>>
>>>>> While SELinux policy is updated, the IDs for object types and such would
>>>>> be changed, and ima_lsm_update_rules would be called.
>>>>>
>>>>> There are no lock applied in ima_lsm_update_rules. If user accesses a
>>>>> file during this time, ima_match_rules will be matching rules based on
>>>>> old SELinux au_seqno resulting in selinux_audit_rule_match returning
>>>>> -ESTALE.
>>>>>
>>>>> However, in ima_match_rules, this error number is not handled, causing
>>>>> IMA to think the LSM rule is also a match, leading to measuring extra
>>>>> files.
>>>
>>> ...
>>>
>>>>> Is this the intended behavior? Or is it a good idea to add a lock for
>>>>> LSM rules during update?
>>>
>>> I'm not the IMA expert here, but a lot of effort has been into the
>>> SELinux code to enable lockless/RCU SELinux policy access and I
>>> *really* don't want to have to backtrack on that.
>>
>> IMA initially updated it's reference to the SELinux label ids lazily.
>> More recently IMA refreshes the LSM label ids based on
>> register_blocking_lsm_notifier(). As a result of commit 9ad6e9cb39c6
>> ("selinux: fix race between old and new sidtab"), -ESTALE is now being
>> returned.
>
> To be clear, are you seeing this only started happening after commit
> 9ad6e9cb39c6? If that is the case, I would suggest a retry loop
> around ima_filter_rule_match() when -ESTALE is returned. I believe
> that should resolve the problem, if not please let us know.
>
Hi Mimi and Paul
It seems that selinux_audit_rule_match has been returning -ESTALE for a
very long time. It dates back to 376bd9cb357ec.
IMA used to have a retry mechanism, but it was removed by b16942455193
("ima: use the lsm policy update notifier"). Maybe we should consider
bring it back or just add a lock in ima_lsm_update_rules().
FYI, once ima received the notification, it starts updating all it's lsm
rules one-by-one. During this time, calling ima_match_rules on any rule
that is not yet updated would return -ESTALE.
--
Best
GUO Zihua
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-17 7:17 ` Guozihua (Scott)
@ 2022-08-17 7:20 ` Guozihua (Scott)
2022-08-17 19:26 ` Mimi Zohar
0 siblings, 1 reply; 10+ messages in thread
From: Guozihua (Scott) @ 2022-08-17 7:20 UTC (permalink / raw)
To: Paul Moore, Mimi Zohar; +Cc: linux-integrity, selinux, xiujianfeng, luhuaxin
On 2022/8/17 15:17, Guozihua (Scott) wrote:
> On 2022/8/16 6:23, Paul Moore wrote:
>> On Sun, Aug 14, 2022 at 2:30 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>
>>> Hi Scott, Paul,
>>>
>>> On Tue, 2022-08-09 at 12:24 -0400, Paul Moore wrote:
>>>> On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott)
>>>> <guozihua@huawei.com> wrote:
>>>>>
>>>>> On 2022/8/8 11:02, Guozihua (Scott) wrote:
>>>>>> Hi Community,
>>>>>>
>>>>>> Recently we discovered a race condition while updating SELinux policy
>>>>>> with IMA lsm rule enabled. Which would lead to extra files being
>>>>>> measured.
>>>>>>
>>>>>> While SELinux policy is updated, the IDs for object types and such
>>>>>> would
>>>>>> be changed, and ima_lsm_update_rules would be called.
>>>>>>
>>>>>> There are no lock applied in ima_lsm_update_rules. If user accesses a
>>>>>> file during this time, ima_match_rules will be matching rules
>>>>>> based on
>>>>>> old SELinux au_seqno resulting in selinux_audit_rule_match returning
>>>>>> -ESTALE.
>>>>>>
>>>>>> However, in ima_match_rules, this error number is not handled,
>>>>>> causing
>>>>>> IMA to think the LSM rule is also a match, leading to measuring extra
>>>>>> files.
>>>>
>>>> ...
>>>>
>>>>>> Is this the intended behavior? Or is it a good idea to add a lock for
>>>>>> LSM rules during update?
>>>>
>>>> I'm not the IMA expert here, but a lot of effort has been into the
>>>> SELinux code to enable lockless/RCU SELinux policy access and I
>>>> *really* don't want to have to backtrack on that.
>>>
>>> IMA initially updated it's reference to the SELinux label ids lazily.
>>> More recently IMA refreshes the LSM label ids based on
>>> register_blocking_lsm_notifier(). As a result of commit 9ad6e9cb39c6
>>> ("selinux: fix race between old and new sidtab"), -ESTALE is now being
>>> returned.
>>
>> To be clear, are you seeing this only started happening after commit
>> 9ad6e9cb39c6? If that is the case, I would suggest a retry loop
>> around ima_filter_rule_match() when -ESTALE is returned. I believe
>> that should resolve the problem, if not please let us know.
>>
>
> Hi Mimi and Paul
>
> It seems that selinux_audit_rule_match has been returning -ESTALE for a
> very long time. It dates back to 376bd9cb357ec.
>
> IMA used to have a retry mechanism, but it was removed by b16942455193
> ("ima: use the lsm policy update notifier"). Maybe we should consider
> bring it back or just add a lock in ima_lsm_update_rules().
>
> FYI, once ima received the notification, it starts updating all it's lsm
> rules one-by-one. During this time, calling ima_match_rules on any rule
> that is not yet updated would return -ESTALE.
>
I mean a retry might still be needed in ima_match_rules(), but not the
ima_lsm_update_rules().
--
Best
GUO Zihua
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-17 7:20 ` Guozihua (Scott)
@ 2022-08-17 19:26 ` Mimi Zohar
2022-08-17 20:49 ` Paul Moore
0 siblings, 1 reply; 10+ messages in thread
From: Mimi Zohar @ 2022-08-17 19:26 UTC (permalink / raw)
To: Guozihua (Scott), Paul Moore
Cc: linux-integrity, selinux, xiujianfeng, luhuaxin
On Wed, 2022-08-17 at 15:20 +0800, Guozihua (Scott) wrote:
> On 2022/8/17 15:17, Guozihua (Scott) wrote:
> > On 2022/8/16 6:23, Paul Moore wrote:
> >> On Sun, Aug 14, 2022 at 2:30 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>
> >>> Hi Scott, Paul,
> >>>
> >>> On Tue, 2022-08-09 at 12:24 -0400, Paul Moore wrote:
> >>>> On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott)
> >>>> <guozihua@huawei.com> wrote:
> >>>>>
> >>>>> On 2022/8/8 11:02, Guozihua (Scott) wrote:
> >>>>>> Hi Community,
> >>>>>>
> >>>>>> Recently we discovered a race condition while updating SELinux policy
> >>>>>> with IMA lsm rule enabled. Which would lead to extra files being
> >>>>>> measured.
> >>>>>>
> >>>>>> While SELinux policy is updated, the IDs for object types and such
> >>>>>> would
> >>>>>> be changed, and ima_lsm_update_rules would be called.
> >>>>>>
> >>>>>> There are no lock applied in ima_lsm_update_rules. If user accesses a
> >>>>>> file during this time, ima_match_rules will be matching rules
> >>>>>> based on
> >>>>>> old SELinux au_seqno resulting in selinux_audit_rule_match returning
> >>>>>> -ESTALE.
> >>>>>>
> >>>>>> However, in ima_match_rules, this error number is not handled,
> >>>>>> causing
> >>>>>> IMA to think the LSM rule is also a match, leading to measuring extra
> >>>>>> files.
> >>>>
> >>>> ...
> >>>>
> >>>>>> Is this the intended behavior? Or is it a good idea to add a lock for
> >>>>>> LSM rules during update?
> >>>>
> >>>> I'm not the IMA expert here, but a lot of effort has been into the
> >>>> SELinux code to enable lockless/RCU SELinux policy access and I
> >>>> *really* don't want to have to backtrack on that.
> >>>
> >>> IMA initially updated it's reference to the SELinux label ids lazily.
> >>> More recently IMA refreshes the LSM label ids based on
> >>> register_blocking_lsm_notifier(). As a result of commit 9ad6e9cb39c6
> >>> ("selinux: fix race between old and new sidtab"), -ESTALE is now being
> >>> returned.
> >>
> >> To be clear, are you seeing this only started happening after commit
> >> 9ad6e9cb39c6? If that is the case, I would suggest a retry loop
> >> around ima_filter_rule_match() when -ESTALE is returned. I believe
> >> that should resolve the problem, if not please let us know.
> >
> > Hi Mimi and Paul
> >
> > It seems that selinux_audit_rule_match has been returning -ESTALE for a
> > very long time. It dates back to 376bd9cb357ec.
> >
> > IMA used to have a retry mechanism, but it was removed by b16942455193
> > ("ima: use the lsm policy update notifier"). Maybe we should consider
> > bring it back or just add a lock in ima_lsm_update_rules().
> >
> > FYI, once ima received the notification, it starts updating all it's lsm
> > rules one-by-one. During this time, calling ima_match_rules on any rule
> > that is not yet updated would return -ESTALE.
>
> I mean a retry might still be needed in ima_match_rules(), but not the
> ima_lsm_update_rules().
Ok. So eventually the LSM label ids are properly updated. Did adding
a retry loop around ima_filter_rule_match(), as Paul suggested, resolve
the problem?
thanks,
Mimi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-17 19:26 ` Mimi Zohar
@ 2022-08-17 20:49 ` Paul Moore
2022-08-18 1:12 ` Guozihua (Scott)
0 siblings, 1 reply; 10+ messages in thread
From: Paul Moore @ 2022-08-17 20:49 UTC (permalink / raw)
To: Mimi Zohar
Cc: Guozihua (Scott), linux-integrity, selinux, xiujianfeng, luhuaxin
On Wed, Aug 17, 2022 at 3:26 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Wed, 2022-08-17 at 15:20 +0800, Guozihua (Scott) wrote:
> > On 2022/8/17 15:17, Guozihua (Scott) wrote:
> > > On 2022/8/16 6:23, Paul Moore wrote:
> > >> On Sun, Aug 14, 2022 at 2:30 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >>>
> > >>> Hi Scott, Paul,
> > >>>
> > >>> On Tue, 2022-08-09 at 12:24 -0400, Paul Moore wrote:
> > >>>> On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott)
> > >>>> <guozihua@huawei.com> wrote:
> > >>>>>
> > >>>>> On 2022/8/8 11:02, Guozihua (Scott) wrote:
> > >>>>>> Hi Community,
> > >>>>>>
> > >>>>>> Recently we discovered a race condition while updating SELinux policy
> > >>>>>> with IMA lsm rule enabled. Which would lead to extra files being
> > >>>>>> measured.
> > >>>>>>
> > >>>>>> While SELinux policy is updated, the IDs for object types and such
> > >>>>>> would
> > >>>>>> be changed, and ima_lsm_update_rules would be called.
> > >>>>>>
> > >>>>>> There are no lock applied in ima_lsm_update_rules. If user accesses a
> > >>>>>> file during this time, ima_match_rules will be matching rules
> > >>>>>> based on
> > >>>>>> old SELinux au_seqno resulting in selinux_audit_rule_match returning
> > >>>>>> -ESTALE.
> > >>>>>>
> > >>>>>> However, in ima_match_rules, this error number is not handled,
> > >>>>>> causing
> > >>>>>> IMA to think the LSM rule is also a match, leading to measuring extra
> > >>>>>> files.
> > >>>>
> > >>>> ...
> > >>>>
> > >>>>>> Is this the intended behavior? Or is it a good idea to add a lock for
> > >>>>>> LSM rules during update?
> > >>>>
> > >>>> I'm not the IMA expert here, but a lot of effort has been into the
> > >>>> SELinux code to enable lockless/RCU SELinux policy access and I
> > >>>> *really* don't want to have to backtrack on that.
> > >>>
> > >>> IMA initially updated it's reference to the SELinux label ids lazily.
> > >>> More recently IMA refreshes the LSM label ids based on
> > >>> register_blocking_lsm_notifier(). As a result of commit 9ad6e9cb39c6
> > >>> ("selinux: fix race between old and new sidtab"), -ESTALE is now being
> > >>> returned.
> > >>
> > >> To be clear, are you seeing this only started happening after commit
> > >> 9ad6e9cb39c6? If that is the case, I would suggest a retry loop
> > >> around ima_filter_rule_match() when -ESTALE is returned. I believe
> > >> that should resolve the problem, if not please let us know.
> > >
> > > Hi Mimi and Paul
> > >
> > > It seems that selinux_audit_rule_match has been returning -ESTALE for a
> > > very long time. It dates back to 376bd9cb357ec.
> > >
> > > IMA used to have a retry mechanism, but it was removed by b16942455193
> > > ("ima: use the lsm policy update notifier"). Maybe we should consider
> > > bring it back or just add a lock in ima_lsm_update_rules().
> > >
> > > FYI, once ima received the notification, it starts updating all it's lsm
> > > rules one-by-one. During this time, calling ima_match_rules on any rule
> > > that is not yet updated would return -ESTALE.
> >
> > I mean a retry might still be needed in ima_match_rules(), but not the
> > ima_lsm_update_rules().
>
> Ok. So eventually the LSM label ids are properly updated. Did adding
> a retry loop around ima_filter_rule_match(), as Paul suggested, resolve
> the problem?
A good long-term solution to this would likely be to add a small
wrapper function for SELinux's security_audit_rule_match() hook (e.g.
loop on selinux_audit_rule_match() when ESTALE is returned) so that
callers wouldn't need to worry about this, but I first want to make
sure that is the problem. If that *is* the problem, I can draft up a
SELinux patch pretty quick.
--
paul-moore.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules
2022-08-17 20:49 ` Paul Moore
@ 2022-08-18 1:12 ` Guozihua (Scott)
0 siblings, 0 replies; 10+ messages in thread
From: Guozihua (Scott) @ 2022-08-18 1:12 UTC (permalink / raw)
To: Paul Moore, Mimi Zohar; +Cc: linux-integrity, selinux, xiujianfeng, luhuaxin
On 2022/8/18 4:49, Paul Moore wrote:
> On Wed, Aug 17, 2022 at 3:26 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>> On Wed, 2022-08-17 at 15:20 +0800, Guozihua (Scott) wrote:
>>> On 2022/8/17 15:17, Guozihua (Scott) wrote:
>>>> On 2022/8/16 6:23, Paul Moore wrote:
>>>>> On Sun, Aug 14, 2022 at 2:30 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>
>>>>>> Hi Scott, Paul,
>>>>>>
>>>>>> On Tue, 2022-08-09 at 12:24 -0400, Paul Moore wrote:
>>>>>>> On Sun, Aug 7, 2022 at 11:19 PM Guozihua (Scott)
>>>>>>> <guozihua@huawei.com> wrote:
>>>>>>>>
>>>>>>>> On 2022/8/8 11:02, Guozihua (Scott) wrote:
>>>>>>>>> Hi Community,
>>>>>>>>>
>>>>>>>>> Recently we discovered a race condition while updating SELinux policy
>>>>>>>>> with IMA lsm rule enabled. Which would lead to extra files being
>>>>>>>>> measured.
>>>>>>>>>
>>>>>>>>> While SELinux policy is updated, the IDs for object types and such
>>>>>>>>> would
>>>>>>>>> be changed, and ima_lsm_update_rules would be called.
>>>>>>>>>
>>>>>>>>> There are no lock applied in ima_lsm_update_rules. If user accesses a
>>>>>>>>> file during this time, ima_match_rules will be matching rules
>>>>>>>>> based on
>>>>>>>>> old SELinux au_seqno resulting in selinux_audit_rule_match returning
>>>>>>>>> -ESTALE.
>>>>>>>>>
>>>>>>>>> However, in ima_match_rules, this error number is not handled,
>>>>>>>>> causing
>>>>>>>>> IMA to think the LSM rule is also a match, leading to measuring extra
>>>>>>>>> files.
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>>> Is this the intended behavior? Or is it a good idea to add a lock for
>>>>>>>>> LSM rules during update?
>>>>>>>
>>>>>>> I'm not the IMA expert here, but a lot of effort has been into the
>>>>>>> SELinux code to enable lockless/RCU SELinux policy access and I
>>>>>>> *really* don't want to have to backtrack on that.
>>>>>>
>>>>>> IMA initially updated it's reference to the SELinux label ids lazily.
>>>>>> More recently IMA refreshes the LSM label ids based on
>>>>>> register_blocking_lsm_notifier(). As a result of commit 9ad6e9cb39c6
>>>>>> ("selinux: fix race between old and new sidtab"), -ESTALE is now being
>>>>>> returned.
>>>>>
>>>>> To be clear, are you seeing this only started happening after commit
>>>>> 9ad6e9cb39c6? If that is the case, I would suggest a retry loop
>>>>> around ima_filter_rule_match() when -ESTALE is returned. I believe
>>>>> that should resolve the problem, if not please let us know.
>>>>
>>>> Hi Mimi and Paul
>>>>
>>>> It seems that selinux_audit_rule_match has been returning -ESTALE for a
>>>> very long time. It dates back to 376bd9cb357ec.
>>>>
>>>> IMA used to have a retry mechanism, but it was removed by b16942455193
>>>> ("ima: use the lsm policy update notifier"). Maybe we should consider
>>>> bring it back or just add a lock in ima_lsm_update_rules().
>>>>
>>>> FYI, once ima received the notification, it starts updating all it's lsm
>>>> rules one-by-one. During this time, calling ima_match_rules on any rule
>>>> that is not yet updated would return -ESTALE.
>>>
>>> I mean a retry might still be needed in ima_match_rules(), but not the
>>> ima_lsm_update_rules().
>>
>> Ok. So eventually the LSM label ids are properly updated. Did adding
>> a retry loop around ima_filter_rule_match(), as Paul suggested, resolve
>> the problem?
>
> A good long-term solution to this would likely be to add a small
> wrapper function for SELinux's security_audit_rule_match() hook (e.g.
> loop on selinux_audit_rule_match() when ESTALE is returned) so that
> callers wouldn't need to worry about this, but I first want to make
> sure that is the problem. If that *is* the problem, I can draft up a
> SELinux patch pretty quick.
>
A retry loop around ima_filter_rule_match() should resolve the problem,
I'll come up with a patch soon. I can try to construct a reproducer for
it at the mean time.
I think it's fine for selinux_audit_rule_match() to return -ESTALE and
let the caller handle it.
--
Best
GUO Zihua
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-08-18 1:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-08 3:02 Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules Guozihua (Scott)
2022-08-08 3:19 ` Guozihua (Scott)
2022-08-09 16:24 ` Paul Moore
2022-08-14 18:30 ` Mimi Zohar
2022-08-15 22:23 ` Paul Moore
2022-08-17 7:17 ` Guozihua (Scott)
2022-08-17 7:20 ` Guozihua (Scott)
2022-08-17 19:26 ` Mimi Zohar
2022-08-17 20:49 ` Paul Moore
2022-08-18 1:12 ` Guozihua (Scott)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.