Re: [PATCH v2] libselinux: add lock callbacks

[Seth Moore <sethmo@google.com>]

On Wed, Jul 14, 2021 at 11:41 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> On Tue, 13 Jul 2021 at 00:30, Seth Moore <sethmo@google.com> wrote:
> >
> > The old mechanism to initialize AVC, avc_init(3), is deprected. This
> > leaves libselinux with no way of guarding the AVC cache when accessed
> > from multiple threads. When applications call access check APIs from
> > multiple threads, the AVC cache may become corrupted.
> >
> > This change adds new callback functions to selinux_set_callback(3).
> > These new callbacks all correspond to the functions that used to be
> > passed via avc_init(3). Multi-threaded applications may set these
> > callbacks to guard the AVC cache against simultaneous access by
> > multiple threads.
> >
> > This change adds the following callbacks:
> >   - SELINUX_CB_ALLOC_LOCK
> >       is invoked to allocate new locks
> >   - SELINUX_CB_GET_LOCK
> >       is invoked to acquire a lock
> >   - SELINUX_CB_RELEASE_LOCK
> >       is invoked to release a previously-acquired lock
> >   - SELINUX_CB_FREE_LOCK
> >       is invoked to free a previosly-allocated lock
> >
> > Signed-off-by: Seth Moore <sethmo@google.com>
>
> Since libselinux 3.2 `avc_init_internal()` uses the SELinux status
> map, via `selinux_status_open()`, by default and by e.g.
> `selinux_check_access()` via `selinux_status_updated()`.
> The status page code is not thread-safe due to the non-thread local
> state variables, like `last_seqno` or `last_policyload`.
> One could mark them with the thread-local storage specifier `__thread`
> (already used within libselinux), but it will result in setenforce-
> and policyload-callbacks for a single event being called multiple
> times for each thread.

Looks like a proper fix for thread safety is a lot more work than I anticipated.
A slightly deeper scan of the code turns up more gotchas/globals:

* `selinux_status_updated` is also reading the global `avc_enforcing`
* `selinux_status_updated` calls `avc_process_policyload`, who then calls
`selinux_flush_class_cache`, which frees global cache outside a lock.

I have a hunch there's more. With that in mind, I'm retracting this patch.
I think a proper one should look quite a bit different (protecting AVC is
not nearly enough).

Thanks for the eyeballs -- I appreciate the feedback/reviews.

Cheers,
Seth

> > diff --git a/libselinux/man/man3/selinux_set_callback.3 b/libselinux/man/man3/selinux_set_callback.3
> > index 75f49b06..f7371504 100644
> > --- a/libselinux/man/man3/selinux_set_callback.3
> > +++ b/libselinux/man/man3/selinux_set_callback.3
> > @@ -116,6 +116,52 @@ The
> >  .I seqno
> >  argument is the current sequential number of the policy generation in the system.
> >  .
> > +.TP
> > +.B SELINUX_CB_ALLOC_LOCK
> > +.BI "void *(*" alloc_lock ") ();"
> > +
> > +This callback is used to allocate a fresh lock for protecting critical sections.
> > +Applications that call selinux library functions from multiple threads must either
> > +perform their own locking or set each of the following:
>
> Maybe mention that these callbacks affect the thread-safety of only a
> subsection of libselinux; the AVC, security_compute_* and
> selinux_check_access interfaces (e.g. the get*con/set*con are
> thread-safe by default).
> Also selinux -> SELinux.