Re: [PATCH v2] libselinux: add lock callbacks

["Christian Göttsche" <cgzones@googlemail.com>]

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.

> 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.