On Wed, Jul 14, 2021 at 11:41 AM Christian Göttsche <cgzones@xxxxxxxxxxxxxx> wrote: > > On Tue, 13 Jul 2021 at 00:30, Seth Moore <sethmo@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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.
