Re: Race conditioned discovered between ima_match_rules and ima_update_lsm_update_rules

[Mimi Zohar <zohar@linux.ibm.com>]

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