Re: [PATCH] Fix build break around __atomic_*() with GCC<4.7

2018-08-13 Thread Tom Cherry via Selinux 
On Mon, Aug 13, 2018 at 1:49 PM Hollis Blanchard
 wrote:
>
> On 08/13/2018 01:45 PM, Tom Cherry wrote:
> > On Mon, Aug 13, 2018 at 12:43 PM Hollis Blanchard
> >  wrote:
> >> The __atomic_* GCC primitives were introduced in GCC 4.7, but Red Hat
> >> Enterprise Linux 6.x (for example) provides GCC 4.4. Tweak the current 
> >> code to
> >> use the (most conservative) __sync_synchronize() primitive provided by 
> >> those
> >> older GCC versions.
> >>
> >> Fixes https://github.com/SELinuxProject/selinux/issues/97
> >>
> >> (Really, no __atomic or __sync operations are needed here at all, since 
> >> POSIX
> >> 4.12 "Memory Synchronization" says pthread_mutex_lock() and
> >> pthread_mutex_unlock() "synchronize memory with respect to other 
> >> threads"...)
> > That section means that pthread_mutex_lock() and
> > pthread_mutex_unlock() will perform an acquire / release operation
> > respectively, so if you're guarding shared data with them, you don't
> > need additional memory synchronization.
> >
> > In this case however, the fast path does not call pthread_mutex_lock()
> > and thus there is no acquire operation.  pthread_mutex_unlock() will
> > perform a release operation in the thread that actually compiled the
> > regex (so technically, we don't actually need the __atomic_store_n),
> > but we still need an acquire operation on the fast path, which is why
> > we need the atomic.
> You really don't. The fast path access will race whether it's "atomic"
> or not. Luckily, it doesn't matter if you get false positives or false
> negatives, because it will be checked for real under the mutex.

It does matter if you get false positives.  False positives do not
lock the mutex and return immediately that the regex has been
compiled.  Without the acquire operation, it's possible that
spec->regex_compiled is true, but the value of spec->regex hasn't been
made visible to the processor executing this thread, which results in
an error.

Keep in mind, this code runs on ARM as well as x86, and ARM doesn't
guarantee strong memory ordering.

>
> Hollis Blanchard
> Mentor Graphics Emulation Division
>
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

Re: [PATCH] Fix build break around __atomic_*() with GCC<4.7

2018-08-13 Thread Tom Cherry via Selinux 
On Mon, Aug 13, 2018 at 12:43 PM Hollis Blanchard
 wrote:
>
> The __atomic_* GCC primitives were introduced in GCC 4.7, but Red Hat
> Enterprise Linux 6.x (for example) provides GCC 4.4. Tweak the current code to
> use the (most conservative) __sync_synchronize() primitive provided by those
> older GCC versions.
>
> Fixes https://github.com/SELinuxProject/selinux/issues/97
>
> (Really, no __atomic or __sync operations are needed here at all, since POSIX
> 4.12 "Memory Synchronization" says pthread_mutex_lock() and
> pthread_mutex_unlock() "synchronize memory with respect to other threads"...)

That section means that pthread_mutex_lock() and
pthread_mutex_unlock() will perform an acquire / release operation
respectively, so if you're guarding shared data with them, you don't
need additional memory synchronization.

In this case however, the fast path does not call pthread_mutex_lock()
and thus there is no acquire operation.  pthread_mutex_unlock() will
perform a release operation in the thread that actually compiled the
regex (so technically, we don't actually need the __atomic_store_n),
but we still need an acquire operation on the fast path, which is why
we need the atomic.

It's a common pattern,
https://en.wikipedia.org/wiki/Double-checked_locking, and always uses
atomics.

The rest of the change looks fine to me though.

Tom

Tom

>
> ---
>  libselinux/src/label_file.h | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
> index 2fa85474..47859baf 100644
> --- a/libselinux/src/label_file.h
> +++ b/libselinux/src/label_file.h
> @@ -351,8 +351,14 @@ static inline int compile_regex(struct saved_data *data, 
> struct spec *spec,
>  * init_routine does not take a parameter, it's not possible
>  * to use, so we generate the same effect with atomics and a
>  * mutex */
> +#ifdef __ATOMIC_RELAXED
> regex_compiled =
> __atomic_load_n(>regex_compiled, __ATOMIC_ACQUIRE);
> +#else
> +   /* GCC <4.7 */
> +   __sync_synchronize();
> +   regex_compiled = spec->regex_compiled;
> +#endif
> if (regex_compiled) {
> return 0; /* already done */
> }
> @@ -360,8 +366,14 @@ static inline int compile_regex(struct saved_data *data, 
> struct spec *spec,
> __pthread_mutex_lock(>regex_lock);
> /* Check if another thread compiled the regex while we waited
>  * on the mutex */
> +#ifdef __ATOMIC_RELAXED
> regex_compiled =
> __atomic_load_n(>regex_compiled, __ATOMIC_ACQUIRE);
> +#else
> +   /* GCC <4.7 */
> +   __sync_synchronize();
> +   regex_compiled = spec->regex_compiled;
> +#endif
> if (regex_compiled) {
> __pthread_mutex_unlock(>regex_lock);
> return 0;
> @@ -404,7 +416,13 @@ static inline int compile_regex(struct saved_data *data, 
> struct spec *spec,
> }
>
> /* Done. */
> +#ifdef __ATOMIC_RELAXED
> __atomic_store_n(>regex_compiled, true, __ATOMIC_RELEASE);
> +#else
> +   /* GCC <4.7 */
> +   spec->regex_compiled = true;
> +   __sync_synchronize();
> +#endif
> __pthread_mutex_unlock(>regex_lock);
> return 0;
>  }
> --
> 2.13.0
>
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[PATCH] libselinux: fix thread safety issues with lookup_common()

2017-07-26 Thread Tom Cherry via Selinux 
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   |  5 -
 libselinux/src/label_file.h   | 32 ++--
 libselinux/src/regex.c| 27 +++
 libselinux/src/regex.h|  7 ++-
 libselinux/src/selinux_internal.h | 32 
 5 files changed, 95 insertions(+), 8 deletions(-)

diff --git a/libselinux/src/label_file.c b/libselinux/src/label_file.c
index f84d470b..560d8c3d 100644
--- a/libselinux/src/label_file.c
+++ b/libselinux/src/label_file.c
@@ -389,10 +389,12 @@ end_arch_check:
spec->prefix_len = prefix_len;
}
 
-   rc = regex_load_mmap(mmap_area, >regex, reg_arch_matches);
+   rc = regex_load_mmap(mmap_area, >regex, reg_arch_matches,
+>regex_compiled);
if (rc < 0)
goto out;
 
+   __pthread_mutex_init(>regex_lock, NULL);
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(>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 
+#include 
 #include 
 
 #include 
@@ -16,6 +17,7 @@
 
 #include "callbacks.h"
 #include "label_internal.h"
+#include "selinux_internal.h"
 
 #define SELINUX_MAGIC_COMPILED_FCONTEXT0xf97cff8a
 
@@ -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(>regex_compiled, __ATOMIC_ACQUIRE);
+   if (regex_compiled) {
return 0; /* already done */
+   }
+
+   __pthread_mutex_lock(>regex_lock);
+   /* Check if another thread compiled the regex while we waited
+* on the mutex */
+   regex_compiled =
+   __atomic_load_n(>regex_compiled, __ATOMIC_ACQUIRE);
+   if (regex_compiled) {
+   __pthread_mutex_unlock(>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(>regex_lock);

[PATCH] libselinux: fix thread safety issues with lookup_common()

2017-07-25 Thread Tom Cherry via Selinux 
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(>regex_lock, NULL);
+   spec->regex_compiled = true;
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(>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 
+#include 
 #include 
 
 #include 
@@ -16,6 +17,7 @@
 
 #include "callbacks.h"
 #include "label_internal.h"
+#include "selinux_internal.h"
 
 #define SELINUX_MAGIC_COMPILED_FCONTEXT0xf97cff8a
 
@@ -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(>regex_compiled, __ATOMIC_ACQUIRE);
+   if (regex_compiled) {
return 0; /* already done */
+   }
+
+   __pthread_mutex_lock(>regex_lock);
+   /* Check if another thread compiled the regex while we waited
+* on the mutex */
+   regex_compiled =
+   __atomic_load_n(>regex_compiled, __ATOMIC_ACQUIRE);
+   if (regex_compiled) {
+   __pthread_mutex_unlock(>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(>regex_lock);
return -1;
}
 
@@ -374,10 +397,13 @@ static inline int compile_regex(struct saved_data *data, 
struct spec *spec,
sizeof(regex_error_format_buffer));
*errbuf = _error_format_buffer[0];
}
+

Re: [PATCH] procattr.c: Use __BIONIC__ instead of __ANDROID__

2017-05-12 Thread Tom Cherry via Selinux 
On Fri, May 12, 2017 at 6:22 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
> On Thu, 2017-05-11 at 16:50 -0700, Tom Cherry via Selinux wrote:
>> This check is not specific to Android devices. If libselinux were
>> used
>> with Bionic on a normal Linux system this check would still be
>> needed.
>>
>> Signed-off-by: Tom Cherry <tomche...@google.com>
>
> Thanks, applied.  This was actually switched from ANDROID to _ANDROID__
>  by nnk in 044f6ef104c8a9d8f42faa8756e71a0525198f5b.  We don't appear
> to have any other uses of __ANDROID__, but we do have a number of uses
> of ANDROID.  Offhand though these other uses of ANDROID don't appear to
> be related to bionic but instead reflect differences in SELinux
> userspace integration in Android vs GNU/Linux.  If however your
> bionic/Linux system integrates SELinux support in an Android-like
> manner (e.g. kernel policy file is /sepolicy, no /etc/selinux/config,
> ...), then you might need to generalize those as well.

Thanks!  I took a look at the other ANDROID ifdefs in the code and I'm not sure
how or if they would need to be changed.  This change is needed to get
libselinux building with bionic on the host which is just the first step.

>
>> ---
>>  libselinux/src/procattr.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
>> index ebc0adec..48dd8aff 100644
>> --- a/libselinux/src/procattr.c
>> +++ b/libselinux/src/procattr.c
>> @@ -22,8 +22,8 @@ static pthread_key_t destructor_key;
>>  static int destructor_key_initialized = 0;
>>  static __thread char destructor_initialized;
>>
>> -#ifndef __ANDROID__
>> -/* Android declares this in unistd.h and has a definition for it */
>> +#ifndef __BIONIC__
>> +/* Bionic declares this in unistd.h and has a definition for it */
>>  static pid_t gettid(void)
>>  {
>>   return syscall(__NR_gettid);

[PATCH] procattr.c: Use __BIONIC__ instead of __ANDROID__

2017-05-11 Thread Tom Cherry via Selinux 
This check is not specific to Android devices. If libselinux were used
with Bionic on a normal Linux system this check would still be needed.

Signed-off-by: Tom Cherry 
---
 libselinux/src/procattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
index ebc0adec..48dd8aff 100644
--- a/libselinux/src/procattr.c
+++ b/libselinux/src/procattr.c
@@ -22,8 +22,8 @@ static pthread_key_t destructor_key;
 static int destructor_key_initialized = 0;
 static __thread char destructor_initialized;
 
-#ifndef __ANDROID__
-/* Android declares this in unistd.h and has a definition for it */
+#ifndef __BIONIC__
+/* Bionic declares this in unistd.h and has a definition for it */
 static pid_t gettid(void)
 {
return syscall(__NR_gettid);
-- 
2.13.0.rc2.291.g57267f2277-goog