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 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