On Mon, Jul 20, 2020 at 9:47 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> 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.
Also, please put a space between while and ( above and put the
semicolon on a line by itself.
I'd also prefer a space between the (void) typecast and various calls
even though it wasn't that way in the existing code.
