Re: unnecessary log output in selinux_status_updated

[Petr Lautrbach <plautrba@redhat.com>]

Stephen Smalley <stephen.smalley.work@gmail.com> writes:

> On Fri, Oct 7, 2022 at 9:53 AM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Stephen Smalley <stephen.smalley.work@gmail.com> writes:
>>
>> > On Fri, Oct 7, 2022 at 4:54 AM Petr Lautrbach <plautrba@redhat.com> 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.

rpm maintainer decided to implement a logging callback [1], and so there's
no need to change this back.

[1] https://github.com/rpm-software-management/rpm/pull/2201

Petr