On Mon, Jul 12, 2021 at 12:29 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > On Fri, Jul 9, 2021 at 9:08 PM 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> > > Hello, > > I am not very familiar with libselinux's callback, but this patch > looks good to me and it implements the solution which was discussed > previously (https://lore.kernel.org/selinux/CAEjxPJ4A7KC=+0vTKNU_Z0K-e9Q6hSfTc5WiNms54EN_C5dCLA@xxxxxxxxxxxxxx/). > > Nevertheless there is a slight issue with the CHECK_LOCK_CALLBACKS > macro: calling "CHECK_LOCK_CALLBACKS();" will expand to "if > (...){...};", with an unnecessary semicolon which is reported by clang > as -Wextra-semi-stmt (for example in > https://github.com/fishilico/selinux/runs/3035178820?check_suite_focus=true). > While I like this check, it seems better if CHECK_LOCK_CALLBACKS was a > function (which would be static inline). Could you please send a > version 2 with this macro replaced by a function? (or I may be wrong, > in which case please explain why using a macro is better than an > inline function). Thanks for looking. I debated making this a function, but in the end liked the idea of the assert being inside the calling function body. However, the call stack should have that info anyway, so... in hindsight perhaps I was overthinking it. I'll rework and submit a new patch that just calls a function. (Which I generally prefer to a macro anyhow). Cheers, Seth > > Thanks, > Nicolas > > > --- > > libselinux/include/selinux/selinux.h | 12 ++++++ > > libselinux/man/man3/selinux_set_callback.3 | 46 ++++++++++++++++++++++ > > libselinux/src/avc_internal.h | 22 +++++++++-- > > libselinux/src/callbacks.c | 13 ++++++ > > 4 files changed, 90 insertions(+), 3 deletions(-) > > > > diff --git a/libselinux/include/selinux/selinux.h b/libselinux/include/selinux/selinux.h > > index ae98a92e..c3c68b3a 100644 > > --- a/libselinux/include/selinux/selinux.h > > +++ b/libselinux/include/selinux/selinux.h > > @@ -166,6 +166,14 @@ __attribute__ ((format(printf, 2, 3))) > > int (*func_setenforce) (int enforcing); > > /* netlink callback for policyload message */ > > int (*func_policyload) (int seqno); > > + /* create a lock and return an opaque pointer to it */ > > + void *(*func_alloc_lock) (void); > > + /* obtain a given lock, blocking if necessary */ > > + void (*func_get_lock) (void *lock); > > + /* release a given lock */ > > + void (*func_release_lock) (void *lock); > > + /* destroy a given lock */ > > + void (*func_free_lock) (void *lock); > > }; > > > > #define SELINUX_CB_LOG 0 > > @@ -173,6 +181,10 @@ __attribute__ ((format(printf, 2, 3))) > > #define SELINUX_CB_VALIDATE 2 > > #define SELINUX_CB_SETENFORCE 3 > > #define SELINUX_CB_POLICYLOAD 4 > > +#define SELINUX_CB_ALLOC_LOCK 5 > > +#define SELINUX_CB_GET_LOCK 6 > > +#define SELINUX_CB_RELEASE_LOCK 7 > > +#define SELINUX_CB_FREE_LOCK 8 > > > > extern union selinux_callback selinux_get_callback(int type); > > extern void selinux_set_callback(int type, union selinux_callback cb); > > 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: > > + > > +.B SELINUX_CB_ALLOC_LOCK > > + > > +.B SELINUX_CB_GET_LOCK > > + > > +.B SELINUX_CB_RELEASE_LOCK > > + > > +.B SELINUX_CB_FREE_LOCK > > + > > +.TP > > +.B SELINUX_CB_GET_LOCK > > +.BI "void (*" get_lock ") (void *" lock ");" > > + > > +This callback acquires the > > +.I lock > > +that was previously allocated with > > +.I alloc_lock. > > +This function must block until the > > +.I lock > > +can be acquired. > > +. > > +.TP > > +.B SELINUX_CB_RELEASE_LOCK > > +.BI "void (*" release_lock ") (void *" lock ");" > > + > > +This callback releases the > > +.I lock > > +that was previously acquired with > > +.I get_lock. > > +. > > +.TP > > +.B SELINUX_CB_FREE_LOCK > > +.BI "void (*" free_lock ") (void *" lock ");" > > + > > +This callback frees the > > +.I lock > > +that was previously allocated with > > +.I alloc_lock. > > +. > > .SH "RETURN VALUE" > > None. > > . > > diff --git a/libselinux/src/avc_internal.h b/libselinux/src/avc_internal.h > > index a9a4aa0b..f23690a1 100644 > > --- a/libselinux/src/avc_internal.h > > +++ b/libselinux/src/avc_internal.h > > @@ -9,6 +9,7 @@ > > #ifndef _SELINUX_AVC_INTERNAL_H_ > > #define _SELINUX_AVC_INTERNAL_H_ > > > > +#include <assert.h> > > #include <stdio.h> > > #include <stdlib.h> > > #include <string.h> > > @@ -112,26 +113,41 @@ static inline void avc_stop_thread(void *thread) > > avc_func_stop_thread(thread); > > } > > > > +#define CHECK_LOCK_CALLBACKS() \ > > + if (avc_func_alloc_lock \ > > + || avc_func_get_lock \ > > + || avc_func_release_lock \ > > + || avc_func_free_lock) { \ > > + assert(avc_func_alloc_lock \ > > + && avc_func_get_lock \ > > + && avc_func_release_lock \ > > + && avc_func_free_lock); \ > > + } > > + > > static inline void *avc_alloc_lock(void) > > { > > + CHECK_LOCK_CALLBACKS(); > > return avc_func_alloc_lock ? avc_func_alloc_lock() : NULL; > > } > > > > static inline void avc_get_lock(void *lock) > > { > > - if (avc_func_get_lock) > > + CHECK_LOCK_CALLBACKS(); > > + if (avc_func_get_lock && lock) > > avc_func_get_lock(lock); > > } > > > > static inline void avc_release_lock(void *lock) > > { > > - if (avc_func_release_lock) > > + CHECK_LOCK_CALLBACKS(); > > + if (avc_func_release_lock && lock) > > avc_func_release_lock(lock); > > } > > > > static inline void avc_free_lock(void *lock) > > { > > - if (avc_func_free_lock) > > + CHECK_LOCK_CALLBACKS(); > > + if (avc_func_free_lock && lock) > > avc_func_free_lock(lock); > > } > > > > diff --git a/libselinux/src/callbacks.c b/libselinux/src/callbacks.c > > index c18ccc54..b635c8d8 100644 > > --- a/libselinux/src/callbacks.c > > +++ b/libselinux/src/callbacks.c > > @@ -9,6 +9,7 @@ > > #include <errno.h> > > #include <selinux/selinux.h> > > #include "callbacks.h" > > +#include "avc_internal.h" > > > > /* default implementations */ > > static int __attribute__ ((format(printf, 2, 3))) > > @@ -95,6 +96,18 @@ selinux_set_callback(int type, union selinux_callback cb) > > case SELINUX_CB_POLICYLOAD: > > selinux_netlink_policyload = cb.func_policyload; > > break; > > + case SELINUX_CB_ALLOC_LOCK: > > + avc_func_alloc_lock = cb.func_alloc_lock; > > + break; > > + case SELINUX_CB_GET_LOCK: > > + avc_func_get_lock = cb.func_get_lock; > > + break; > > + case SELINUX_CB_RELEASE_LOCK: > > + avc_func_release_lock = cb.func_release_lock; > > + break; > > + case SELINUX_CB_FREE_LOCK: > > + avc_func_free_lock = cb.func_free_lock; > > + break; > > } > > } > > > > -- > > 2.32.0.93.g670b81a890-goog > > >
