On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
<mike.palmiotto@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jul 14, 2020 at 5:03 PM Stephen Smalley
> <stephen.smalley.work@xxxxxxxxx> wrote:
> >
> > On Tue, Jul 14, 2020 at 4:35 PM Mike Palmiotto
> > <mike.palmiotto@xxxxxxxxxxxxxxx> wrote:
> > >
> > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > /selinux/status") introduced the selinux_status page mechanism, which
> > > allows for mmap()'ing of selinux status state as a replacement for
> > > avc_netlink.
> > >
> > > The mechanism was initially intended for use by userspace object
> > > managers which were calculating access decisions in-tree 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 moves the netlink notice logic out into convenience
> > > functions, which are then called by the sestatus code. Since sestatus
> > > uses netlink as a fallback, we can change the avc_netlink_check_nb()
> > > call in avc_has_perm_noaudit to check the status page if it is
> > > available. If it is not, we fall back to
> >
> > Missing word/phrase here.
>
> Oops. I'll go ahead and fix that, thank...
>
> > Also you need to do more than just replace
> > this one call or selinux_status_updated() will do nothing unless the
> > application has explicitly done a selinux_status_open() itself, e.g.
> > avc_netlink_open -> selinux_status_open, avc_netlink_close ->
> > selinux_status_close, deal with other avc_netlink_* calls including
> > the multi-threaded case.
>
> That was by design. I figured for this version of the patch, we could
> just introduce the ability to use sestatus instead of netlink (if
> open).
In its current form, your patch replaces one call in
avc_has_perm_noaudit() to avc_netlink_check_nb() with a call to
selinux_status_updated(). With that change alone, for existing
callers of avc_has_perm_noaudit() that do not themselves call
selinux_status_open(), selinux_status_updated() will find that the
status page is NULL and will return immediately with an error. It
won't fall back to probing the netlink socket. So you either need to
test for an error return and fall back yourself to
avc_netlink_check_nb(), or change selinux_status_updated() to do that,
or replace the use of avc_netlink_open/loop/close in avc.c with
selinux_status_open/close. Likewise for selinux_check_access().
> Do you think we should go ahead and completely swap in sestatus? I was
> just worried about breaking userspace object managers that are
> currently using netlink threads by default, for instance
> systemd-dbusd. I can spend some more time getting those to work with
> the status page if you think that's worthwhile.
I'd be interested in understanding the impact of such a change on
existing userspace object managers. If we can switch the default
behavior for applications that are not explicitly using
avc_netlink_*() interfaces themselves (e.g. they are only using
selinux_check_access or avc_has_perm), then that would be beneficial
since I think it fully removes the need for a system call on the AVC
cache-hit code path.
> > Finally, I don't think you need to sanitize
> > the enforcing value from the kernel; it takes care of that itself
> > these days and no point in fixing it up for old kernels now.
>
> Very good to know, thanks! I'll update in the next version.
