On Tue, 2017-07-25 at 15:59 -0700, Tom Cherry via Selinux wrote:
> There are two problems with lookup_common() and therefore
> selabel_lookup() and related functions that this patch fixes:
>
> 1) A race with the lazy compilation of regexes. Since the struct
> regex_data is allocated and assigned immediately to the parent struct
> spec, it's possible for a second thread to see that this pointer is
> non-NULL before the regex compilation has finished. This typically
> results in a -1 return from selabel_lookup() with ENOENT as errno.
>
> This is fixed by adding synchronization in compile_regex().
>
> 2) A race with PCRE2 regex_match(). A struct pcre2_match_data is
> created once and used for all regex matches for a given regex. This
> is problematic if two threads are attempting to evaluate the same
> regex simultaneously. This typically results in a successful return
> from selabel_lookup() but with an erroneous selabel.
>
> This is fixed by adding a pthread_mutex within regex_match() for
> PCRE2. Note, on my system, creating new matchdata takes roughly an
> order of magnitude more time than locking a non-contended
> pthread_mutex. I don't believe programs will have enough contention
> on this lock to justify that cost.
>
> Bug: 63861738
> Test: ueventd unit tests
> Change-Id: I13bf782d81d0a0b896d444e396f307ad0dbacb6a
> ---
> libselinux/src/label_file.c | 3 +++
> libselinux/src/label_file.h | 32
> ++++++++++++++++++++++++++++++--
> libselinux/src/regex.c | 18 ++++++++++++++++--
> libselinux/src/selinux_internal.h | 32
> ++++++++++++++++++++++++++++++++
> 4 files changed, 81 insertions(+), 4 deletions(-)
>
> diff --git a/libselinux/src/label_file.c
> b/libselinux/src/label_file.c
> index f84d470b..6300758e 100644
> --- a/libselinux/src/label_file.c
> +++ b/libselinux/src/label_file.c
> @@ -393,6 +393,8 @@ end_arch_check:
> if (rc < 0)
> goto out;
>
> + __pthread_mutex_init(&spec->regex_lock, NULL);
> + spec->regex_compiled = true;
This isn't necessarily true in the PCRE2 case; the file_contexts.bin
file may have been generated without the precompiled regexes (via
sefcontext_compile -r) or the regex architecture string may not match
and thus the precompiled regexes cannot be used. You probably need to
pass &spec->regex_compiled to regex_load_mmap() and let it set the bool
appropriately.
> data->nspec++;
> }
>
> @@ -810,6 +812,7 @@ static void closef(struct selabel_handle *rec)
> free(spec->lr.ctx_trans);
> free(spec->lr.ctx_raw);
> regex_data_free(spec->regex);
> + __pthread_mutex_destroy(&spec->regex_lock);
> if (spec->from_mmap)
> continue;
> free(spec->regex_str);
> diff --git a/libselinux/src/label_file.h
> b/libselinux/src/label_file.h
> index de804aed..aa576d8e 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -2,6 +2,7 @@
> #define _SELABEL_FILE_H_
>
> #include <errno.h>
> +#include <pthread.h>
> #include <string.h>
>
> #include <sys/stat.h>
> @@ -16,6 +17,7 @@
>
> #include "callbacks.h"
> #include "label_internal.h"
> +#include "selinux_internal.h"
>
> #define SELINUX_MAGIC_COMPILED_FCONTEXT 0xf97cff8a
>
> @@ -42,6 +44,8 @@ struct spec {
> char *regex_str; /* regular expession string for
> diagnostics */
> char *type_str; /* type string for diagnostic
> messages */
> struct regex_data * regex; /* backend dependent regular
> expression data */
> + bool regex_compiled; /* bool to indicate if the regex is
> compiled */
> + pthread_mutex_t regex_lock; /* lock for lazy compilation of
> regex */
> mode_t mode; /* mode format value */
> int matches; /* number of matching pathnames
> */
> int stem_id; /* indicates which stem-
> compression item */
> @@ -339,9 +343,27 @@ static inline int compile_regex(struct
> saved_data *data, struct spec *spec,
> struct stem *stem_arr = data->stem_arr;
> size_t len;
> int rc;
> -
> - if (spec->regex)
> + bool regex_compiled;
> +
> + /* We really want pthread_once() here, but since its
> + * init_routine does not take a parameter, it's not possible
> + * to use, so we generate the same effect with atomics and a
> + * mutex */
> + regex_compiled =
> + __atomic_load_n(&spec->regex_compiled,
> __ATOMIC_ACQUIRE);
> + if (regex_compiled) {
> return 0; /* already done */
> + }
> +
> + __pthread_mutex_lock(&spec->regex_lock);
> + /* Check if another thread compiled the regex while we
> waited
> + * on the mutex */
> + regex_compiled =
> + __atomic_load_n(&spec->regex_compiled,
> __ATOMIC_ACQUIRE);
> + if (regex_compiled) {
> + __pthread_mutex_unlock(&spec->regex_lock);
> + return 0;
> + }
>
> /* Skip the fixed stem. */
> reg_buf = spec->regex_str;
> @@ -354,6 +376,7 @@ static inline int compile_regex(struct saved_data
> *data, struct spec *spec,
> if (!anchored_regex) {
> if (errbuf)
> *errbuf = "out of memory";
> + __pthread_mutex_unlock(&spec->regex_lock);
> return -1;
> }
>
> @@ -374,10 +397,13 @@ static inline int compile_regex(struct
> saved_data *data, struct spec *spec,
> sizeof(regex_error_format_bu
> ffer));
> *errbuf = ®ex_error_format_buffer[0];
> }
> + __pthread_mutex_unlock(&spec->regex_lock);
> return -1;
> }
>
> /* Done. */
> + __atomic_store_n(&spec->regex_compiled, true,
> __ATOMIC_RELEASE);
> + __pthread_mutex_unlock(&spec->regex_lock);
> return 0;
> }
>
> @@ -439,6 +465,8 @@ static inline int process_line(struct
> selabel_handle *rec,
> /* process and store the specification in spec. */
> spec_arr[nspec].stem_id = find_stem_from_spec(data, regex);
> spec_arr[nspec].regex_str = regex;
> + __pthread_mutex_init(&spec_arr[nspec].regex_lock, NULL);
> + spec_arr[nspec].regex_compiled = false;
>
> spec_arr[nspec].type_str = type;
> spec_arr[nspec].mode = 0;
> diff --git a/libselinux/src/regex.c b/libselinux/src/regex.c
> index ec1b0c4a..97f0fdb0 100644
> --- a/libselinux/src/regex.c
> +++ b/libselinux/src/regex.c
> @@ -1,10 +1,12 @@
> #include <assert.h>
> +#include <pthread.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <string.h>
>
> #include "regex.h"
> #include "label_file.h"
> +#include "selinux_internal.h"
>
> #ifdef USE_PCRE2
> #define REGEX_ARCH_SIZE_T PCRE2_SIZE
> @@ -63,6 +65,7 @@ struct regex_data {
> * pattern in pcre2
> */
> pcre2_match_data *match_data;
> + pthread_mutex_t match_mutex;
> };
>
> int regex_prepare_data(struct regex_data **regex, char const
> *pattern_string,
> @@ -199,6 +202,7 @@ void regex_data_free(struct regex_data *regex)
> pcre2_code_free(regex->regex);
> if (regex->match_data)
> pcre2_match_data_free(regex->match_data);
> + __pthread_mutex_destroy(®ex->match_mutex);
> free(regex);
> }
> }
> @@ -206,9 +210,11 @@ void regex_data_free(struct regex_data *regex)
> int regex_match(struct regex_data *regex, char const *subject, int
> partial)
> {
> int rc;
> + __pthread_mutex_lock(®ex->match_mutex);
> rc = pcre2_match(
> regex->regex, (PCRE2_SPTR)subject,
> PCRE2_ZERO_TERMINATED, 0,
> partial ? PCRE2_PARTIAL_SOFT : 0, regex->match_data,
> NULL);
> + __pthread_mutex_unlock(®ex->match_mutex);
> if (rc > 0)
> return REGEX_MATCH;
> switch (rc) {
> @@ -244,6 +250,14 @@ int regex_cmp(struct regex_data *regex1, struct
> regex_data *regex2)
> return SELABEL_EQUAL;
> }
>
> +struct regex_data *regex_data_create(void)
> +{
> + struct regex_data *regex_data =
> + (struct regex_data *)calloc(1, sizeof(struct
> regex_data));
> + __pthread_mutex_init(®ex_data->match_mutex, NULL);
> + return regex_data;
> +}
> +
> #else // !USE_PCRE2
> char const *regex_arch_string(void)
> {
> @@ -472,13 +486,13 @@ int regex_cmp(struct regex_data *regex1, struct
> regex_data *regex2)
> return SELABEL_EQUAL;
> }
>
> -#endif
> -
> struct regex_data *regex_data_create(void)
> {
> return (struct regex_data *)calloc(1, sizeof(struct
> regex_data));
> }
>
> +#endif
> +
> void regex_format_error(struct regex_error_data const *error_data,
> char *buffer,
> size_t buf_size)
> {
> diff --git a/libselinux/src/selinux_internal.h
> b/libselinux/src/selinux_internal.h
> index 54949c13..dfc421cc 100644
> --- a/libselinux/src/selinux_internal.h
> +++ b/libselinux/src/selinux_internal.h
> @@ -144,6 +144,38 @@ extern int selinux_page_size hidden;
> pthread_setspecific(KEY, VALUE); \
> } while (0)
>
> +/* selabel_lookup() is only thread safe if we're compiled with
> pthreads */
> +
> +#pragma weak pthread_mutex_init
> +#pragma weak pthread_mutex_destroy
> +#pragma weak pthread_mutex_lock
> +#pragma weak pthread_mutex_unlock
> +
> +#define __pthread_mutex_init(LOCK, ATTR) \
> + do {
> \
> + if (pthread_mutex_init != NULL)
> \
> + pthread_mutex_init(LOCK, ATTR);
> \
> + } while (0)
> +
> +#define __pthread_mutex_destroy(LOCK)
> \
> + do {
> \
> + if (pthread_mutex_destroy != NULL) \
> + pthread_mutex_destroy(LOCK);
> \
> + } while (0)
> +
> +#define __pthread_mutex_lock(LOCK) \
> + do {
> \
> + if (pthread_mutex_lock != NULL)
> \
> + pthread_mutex_lock(LOCK); \
> + } while (0)
> +
> +#define __pthread_mutex_unlock(LOCK)
> \
> + do {
> \
> + if (pthread_mutex_unlock != NULL) \
> + pthread_mutex_unlock(LOCK); \
> + } while (0)
> +
> +
> #define SELINUXDIR "/etc/selinux/"
> #define SELINUXCONFIG SELINUXDIR "config"
>
