On Fri, Oct 7, 2022 at 9:53 AM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote:
>
> Stephen Smalley <stephen.smalley.work@xxxxxxxxx> writes:
>
> > On Fri, Oct 7, 2022 at 4:54 AM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >>
> >> Commit 05bdc03130d74 ("libselinux: use kernel status page by default") changed
> >> selinux_status_updated() so that it calls avc_process_policyload() and
> >> avc_process_setenforce() and both functions call avc_log() and avc_log() logs to
> >> stderr by default. So when a process like `rpm` checks whether there was a
> >> change, it gets output on stderr which previously wasn't there.
> >>
> >>
> >> Before this change:
> >> >>> from selinux import *
> >> >>> selinux_status_open(0);
> >> 0
> >> >>>
> >> >>> selinux_status_updated();
> >> 0
> >> >>> selinux_mkload_policy(0);
> >> 0
> >> >>> selinux_status_updated();
> >> 1
> >>
> >> Current version:
> >> >>> from selinux import *
> >> elinux_status_updated();
> >> selinux_mkload_policy(0);
> >> selinux_status_updated();
> >> >>> selinux_status_open(0);
> >> 0
> >> >>> selinux_status_updated();
> >> 0
> >> >>> selinux_mkload_policy(0);
> >> 0
> >> >>> selinux_status_updated();
> >> uavc: op=load_policy lsm=selinux seqno=2 res=11
> >>
> >>
> >> The calling process could set its callback but it seems unnecessarily
> >> complicated just for selinux_status_updated() which is supposed to check whether
> >> something has changed or not. Also processing events in this function seems to
> >> be unnecessary.
> >>
> >> It looks like the reason for the new code added to selinux_status_updated() is
> >> that there were several avc_netlink_check_nb() calls replaced by
> >> selinux_status_updated(). Given the problem described above, I don't think it's
> >> correct and I would like to change selinux_status_updated() back and use another
> >> mechanism that would help with the replacement.
> >>
> >>
> >> So what do you think about it?
> >
> > The goal was to switch the AVC and all of its users (e.g.
> > selinux_check_access) over to using the much more efficient SELinux
> > kernel status page mechanism for setenforce and policy load
> > notifications on newer kernels instead of the SELinux netlink
> > mechanism (which imposed extra syscall overhead on the critical path).
> >
> > Understand your concern but unsure as to whether we can just change
> > selinux_status_updated() back now.
> > Would require an audit of all users of selinux_status_updated(), both
> > direct and indirect, to ensure that none of them are relying on this
> > behavior. We can obviously fix the callers within libselinux but
> > addressing external callers is more problematic and is arguably an ABI
> > change. Would need to look at all users of the AVC,
> > selinux_check_access(), etc.
> > This change happened 2 years ago so I have to wonder why it is only
> > coming up now?
>
> Nobody has noticed it.
>
> 83 avc_log(SELINUX_POLICYLOAD,
> 84 "%s: op=load_policy lsm=selinux seqno=%u res=1",
> 85 avc_prefix, seqno);
>
> There's missing '\n' and so this message is sooner or later overwritten by
> something else, see
> https://bugzilla.redhat.com/show_bug.cgi?id=2123637 and
> https://bugzilla.redhat.com/show_bug.cgi?id=2123719
Ok, regardless, we need to ensure that changing it won't break
systemd, dbus, or any other userspace object managers.
