nptl: Fix abort in case of set*id failure
Commit Message
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
On 04/28/2014 02:03 PM, Florian Weimer wrote:
> * nptl/tst-setuid3.c: New file.
And here is the new test case.
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.
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.
@@ -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
@@ -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;
@@ -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. */
};
@@ -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;
@@ -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);