diff mbox

nptl: Fix abort in case of set*id failure

Message ID 20140428120545.20B0643994596@oldenburg.str.redhat.com
State New
Headers show

Commit Message

Florian Weimer April 28, 2014, 12:03 p.m. UTC 
If a call to the set*id functions fails in a multi-threaded program,
the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
was triggered.

We address by checking that all calls to set*id on all threads give
the same result, and only abort if we see success followed by failure
(or vice versa).
---
 nptl/Makefile        |  1 +
 nptl/allocatestack.c | 26 +++++++++++++++++++++++++-
 nptl/descr.h         |  1 +
 nptl/nptl-init.c     |  9 ++++++---
 nptl/pthreadP.h      |  2 ++
 6 files changed, 46 insertions(+), 4 deletions(-)

2014-04-28  Florian Weimer  <fweimer@redhat.com>

	* nptl/pthreadP.h (__nptl_setxid_error): Declare function.
	* nptl/allocatestack.c (__nptl_setxid_error): New function.
	(__nptl_setxid): Initialize error member.  Call
	__nptl_setxid_error.
	* nptl/nptl-init.c (sighandler_setxid): Call __nptl_setxid_error.
	* nptl/descr.h (struct xid_command): Add error member.
	* nptl/tst-setuid3.c: New file.
	* nptl/Makefile (tests): Add it.

Comments

Florian Weimer April 28, 2014, 12:11 p.m. UTC | #1 
On 04/28/2014 02:03 PM, Florian Weimer wrote:

> 	* nptl/tst-setuid3.c: New file.

And here is the new test case.
Ondřej Bílka April 28, 2014, 12:50 p.m. UTC | #2 
On Mon, Apr 28, 2014 at 02:03:02PM +0200, Florian Weimer wrote:
> If a call to the set*id functions fails in a multi-threaded program,
> the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
> was triggered.
> 
> We address by checking that all calls to set*id on all threads give
> the same result, and only abort if we see success followed by failure
> (or vice versa).

A code itself makes sense. However I am not familiar with nptl enough to decide
if its proper solution, like why there is not a race condition if other
thread calls setuid that succeeds followed by setuid that fails.
Florian Weimer April 28, 2014, 1:06 p.m. UTC | #3 
On 04/28/2014 02:50 PM, Ondřej Bílka wrote:
> On Mon, Apr 28, 2014 at 02:03:02PM +0200, Florian Weimer wrote:
>> If a call to the set*id functions fails in a multi-threaded program,
>> the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
>> was triggered.
>>
>> We address by checking that all calls to set*id on all threads give
>> the same result, and only abort if we see success followed by failure
>> (or vice versa).
>
> A code itself makes sense. However I am not familiar with nptl enough to decide
> if its proper solution, like why there is not a race condition if other
> thread calls setuid that succeeds followed by setuid that fails.

There is supposed to be locking to prevent this.  I'm not entirely sure 
if it is sufficient, but my additions do not make things worse.
diff mbox

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index 2876224..5ee064c 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -269,6 +269,7 @@  tests = tst-typesizes \
 	tst-abstime \
 	tst-vfork1 tst-vfork2 tst-vfork1x tst-vfork2x \
 	tst-getpid1 tst-getpid2 tst-getpid3 \
+	tst-setuid3 \
 	tst-initializers1 $(patsubst %,tst-initializers1-%,c89 gnu89 c99 gnu99)
 xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \
 	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 1e22f7d..2d855bf 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -1061,6 +1061,25 @@  setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
     return 0;
 }
 
+/* Check for consistency across set*id system call results.  The abort
+   should not happen as long as all privileges changes happen through
+   the glibc wrappers.  ERROR must be 0 (no error) or an errno
+   code.  */
+void
+attribute_hidden
+__nptl_setxid_error (struct xid_command *cmdp, int error)
+{
+  do
+    {
+      int olderror = cmdp->error;
+      if (olderror == error)
+	break;
+      if (olderror != -1)
+	/* Mismatch between current and previous results.  */
+	abort ();
+    }
+  while (atomic_compare_and_exchange_bool_acq (&cmdp->error, error, -1));
+}
 
 int
 attribute_hidden
@@ -1072,6 +1091,7 @@  __nptl_setxid (struct xid_command *cmdp)
 
   __xidcmd = cmdp;
   cmdp->cntr = 0;
+  cmdp->error = -1;
 
   struct pthread *self = THREAD_SELF;
 
@@ -1157,9 +1177,13 @@  __nptl_setxid (struct xid_command *cmdp)
 				 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
   if (INTERNAL_SYSCALL_ERROR_P (result, err))
     {
-      __set_errno (INTERNAL_SYSCALL_ERRNO (result, err));
+      int error = INTERNAL_SYSCALL_ERRNO (result, err);
+      __nptl_setxid_error (cmdp, error);
+      __set_errno (error);
       result = -1;
     }
+  else
+    __nptl_setxid_error (cmdp, 0);
 
   lll_unlock (stack_cache_lock, LLL_PRIVATE);
   return result;
diff --git a/nptl/descr.h b/nptl/descr.h
index 61d57d5..6738591 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -100,6 +100,7 @@  struct xid_command
   int syscall_no;
   long int id[3];
   volatile int cntr;
+  volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
 };
 
 
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 2796dc5..86d9d77 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -249,9 +249,12 @@  sighandler_setxid (int sig, siginfo_t *si, void *ctx)
   result = INTERNAL_SYSCALL_NCS (__xidcmd->syscall_no, err, 3, __xidcmd->id[0],
 				 __xidcmd->id[1], __xidcmd->id[2]);
   if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
-    /* Safety check.  This should never happen if the setxid system
-       calls are only ever called through their glibc wrappers.  */
-    abort ();
+    {
+      int error = INTERNAL_SYSCALL_ERRNO (result, err);
+      __nptl_setxid_error (__xidcmd, error);
+    }
+  else
+    __nptl_setxid_error (__xidcmd, 0);
 
   /* Reset the SETXID flag.  */
   struct pthread *self = THREAD_SELF;
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 197401a..94e7890 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -578,6 +578,8 @@  extern void _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer
 
 extern void __nptl_deallocate_tsd (void) attribute_hidden;
 
+extern void __nptl_setxid_error (struct xid_command *cmdp, int error)
+  attribute_hidden;
 extern int __nptl_setxid (struct xid_command *cmdp) attribute_hidden;
 #ifndef SHARED
 extern void __nptl_set_robust (struct pthread *self);