On Thu, Jul 16, 2020 at 5:06 PM Mike Palmiotto
<mike.palmiotto@xxxxxxxxxxxxxxx> wrote:
>
> Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> /selinux/status") introduced the sestatus mechanism, which allows for
> mmap()'ing of the kernel status page as a replacement for avc_netlink.
>
> The mechanism was initially intended for use by userspace object
> managers which were calculating access decisions within their
> application and did not rely on the libselinux AVC implementation. In
> order to properly make use of sestatus within avc_has_perm(), the status
> mechanism needs to properly set avc internals during status events;
> else, avc_enforcing is never updated upon sestatus changes.
>
> This commit introduces a new selinux_status_loop() function, which
> replaces the default netlink-equivalent, avc_netlink_loop(). The
> function watches the kernel status page until an error occurs, at which
> point it will close the status page and exit the thread.In the event
> that the status page cannot be opened, the thread will continue to
> function as before by using a fallback netlink socket.
>
> This allows us to replace the call to avc_netlink_open() in
> avc_init_internal() with a call to selinux_status_open() and remove the
> avc_netlink_check_nb() call from the critical code path in
> avc_has_perm_noaudit(), as well as selinux_check_access().
>
> Userspace object managers wanting a netlink socket can call
> avc_netlink_acquire_fd() to open a netlink socket if there is not one
> open already.
>
> Update the manpage to reflect the new selinux_status_loop() and
> avc_netlink_acquire_fd() functionality.
>
> Signed-off-by: Mike Palmiotto <mike.palmiotto@xxxxxxxxxxxxxxx>
A few minor comments. First, the subject line should be changed to
reflect the fact that you are changing it to use the SELinux kernel
status page whenever it is available, not just if it is already open.
Second, I'm unclear on the need/benefit of exporting
selinux_status_loop() as a public API/ABI of libselinux. The
difference from avc_netlink_loop() I think is that one requires access
to functions/state private to libselinux while the other only uses
other public APIs/ABIs. Also selinux_status_loop() is a pretty
trivial function. So unless there is a compelling reason to export
selinux_status_loop(), I wouldn't make it a public interface. A
couple of comments on the code below.
> diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
> index b4648b2d..e36a9a53 100644
> --- a/libselinux/src/avc.c
> +++ b/libselinux/src/avc.c
> @@ -557,9 +557,10 @@ void avc_destroy(void)
>
> avc_get_lock(avc_lock);
>
> + selinux_status_close();
> +
> if (avc_using_threads)
> - avc_stop_thread(avc_netlink_thread);
> - avc_netlink_close();
> + avc_stop_thread(avc_status_thread);
Shouldn't we perform the selinux_status_close() after stopping the thread?
> diff --git a/libselinux/src/libselinux.map b/libselinux/src/libselinux.map
> index 2a368e93..8d8d8fd2 100644
> --- a/libselinux/src/libselinux.map
> +++ b/libselinux/src/libselinux.map
> @@ -203,6 +203,7 @@ LIBSELINUX_1.0 {
> selinux_status_close;
> selinux_status_deny_unknown;
> selinux_status_getenforce;
> + selinux_status_loop;
> selinux_status_open;
> selinux_status_policyload;
> selinux_status_updated;
If we were going to add a new API/ABI, it would go in a new version
for the map, tagged with whatever version it would first appear in
e.g. LIBSELINUX_3.2. But I don't think we need it exported at all.
> diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
> index 86267ff8..6ecb6337 100644
> --- a/libselinux/src/sestatus.c
> +++ b/libselinux/src/sestatus.c
> @@ -131,7 +141,6 @@ int selinux_status_updated(void)
> int selinux_status_getenforce(void)
> {
> uint32_t seqno;
> - uint32_t enforcing;
>
> if (selinux_status == NULL) {
> errno = EINVAL;
> @@ -149,11 +158,11 @@ int selinux_status_getenforce(void)
> do {
> seqno = read_sequence(selinux_status);
>
> - enforcing = selinux_status->enforcing;
> + avc_enforcing = selinux_status->enforcing;
>
> } while (seqno != read_sequence(selinux_status));
>
> - return enforcing ? 1 : 0;
> + return avc_enforcing;
> }
I'm not sure this should update avc_enforcing at all, but if it does,
it should only do so once (e.g. after the loop, not within it), only
if the value actually changed, and it should use
avc_process_setenforce() to do it so that it honors avc_setenforce,
resets the AVC, and calls any callbacks. Otherwise that will never
happen if a selinux_status_getenforce() call occurs prior to
selinux_status_updated() after a setenforce operation.
> @@ -316,6 +325,23 @@ error:
> return -1;
> }
>
> +/*
> + * selinux_status_loop
> + *
> + * Run routine for checking kernel status page in a listening thread.
> + * Falls back on netlink socket in the event of failure to open status page.
> + */
> +void selinux_status_loop(void)
> +{
> + /* Check kernel status page until error occurs */
> + while(selinux_status_updated() >= 0);
> +
> + avc_log(SELINUX_ERROR,
> + "%s: status thread terminating due to error: %d (%s)\n",
> + avc_prefix, errno, strerror(errno));
> + selinux_status_close();
Should we really be closing it here or leaving that to the caller? We
are closing it anyway in the destroy function.
