| Message ID | 20210712223013.2165325-1-sethmo@google.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2] libselinux: add lock callbacks | expand |
On Tue, Jul 13, 2021 at 12:30 AM 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> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org> Thanks, Nicolas > --- > libselinux/include/selinux/selinux.h | 12 ++++++ > libselinux/man/man3/selinux_set_callback.3 | 46 ++++++++++++++++++++++ > libselinux/src/avc_internal.h | 20 ++++++++-- > libselinux/src/callbacks.c | 13 ++++++ > 4 files changed, 88 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..1f8e0102 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,39 @@ static inline void avc_stop_thread(void *thread) > avc_func_stop_thread(thread); > } > > +static inline void check_lock_callbacks(void) > +{ > + 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 >
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.
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.
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..1f8e0102 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,39 @@ static inline void avc_stop_thread(void *thread) avc_func_stop_thread(thread); } +static inline void check_lock_callbacks(void) +{ + 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; } }
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> --- libselinux/include/selinux/selinux.h | 12 ++++++ libselinux/man/man3/selinux_set_callback.3 | 46 ++++++++++++++++++++++ libselinux/src/avc_internal.h | 20 ++++++++-- libselinux/src/callbacks.c | 13 ++++++ 4 files changed, 88 insertions(+), 3 deletions(-)