[musl] synccall patches

[Markus Wichmann <nullplan@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Hi all,

fittingly for Reformation Day, I chose to actually send in some patches
today, rather than provide comments. And fittingly for Halloween, they
concern a scary part of musl, namely __synccall().

First I noticed that the initial return value set in __setxid() is not a
valid return value for the functions that call it. Those are only
allowed to return 0 or -1, not 1. That is easily fixed.

Then I noticed that __synccall() has a logic bug explained in the commit
message. It wouldn't be a bug if semaphores had any kind of preference
for the thread that has waited longest or something, but musl does not
implement something like this. And the kernel explicitly documents that
FUTEX_WAKE will wake a random thread.

At first I thought the best solution would be to forego the serialized
execution of the callback; just release all threads in line 97 (and then
fix the callbacks to cope with parallel execution). But BSD semaphores
have no way to do that, and a mass-release loop like the one on line 110
would have the same issue as given here. It would only be less likely to
happen, with the likelihood rising with the number of threads. So a new
semaphore it is.

Ciao,
Markus

[-- Attachment #2: 0001-Initialize-setxid-retval-to-valid-value.patch --]
[-- Type: text/x-diff, Size: 1381 bytes --]

From 44e97fb238670e83eff31c3894cc8a024cef051c Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Tue, 31 Oct 2023 16:59:47 +0100
Subject: [PATCH 1/2] Initialize setxid retval to valid value.

If __synccall() fails to capture all threads because tkill fails for
some reason other than EAGAIN, then the callback given will never be
executed, so nothing will ever overwrite the initial value. So that is
the value that will be returned from the function. The previous setting
of 1 is not a valid value for setuid() et al. to return.

I chose -EAGAIN since I don't know the reason the synccall failed ahead
of time, but EAGAIN is a specified error code for a possibly temporary
failure in setuid().
---
 src/unistd/setxid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/unistd/setxid.c b/src/unistd/setxid.c
index 487c1a16..0a2d9d12 100644
--- a/src/unistd/setxid.c
+++ b/src/unistd/setxid.c
@@ -28,7 +28,7 @@ int __setxid(int nr, int id, int eid, int sid)
 {
 	/* ret is initially nonzero so that failure of the first thread does not
 	 * trigger the safety kill above. */
-	struct ctx c = { .nr = nr, .id = id, .eid = eid, .sid = sid, .ret = 1 };
+	struct ctx c = { .nr = nr, .id = id, .eid = eid, .sid = sid, .ret = -EAGAIN };
 	__synccall(do_setxid, &c);
 	return __syscall_ret(c.ret);
 }
--
2.39.2


[-- Attachment #3: 0002-synccall-Add-exit_sem.patch --]
[-- Type: text/x-diff, Size: 2469 bytes --]

From f0793c5565b0670fa5283edaaadf8fbdb7f197be Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Tue, 31 Oct 2023 17:03:44 +0100
Subject: [PATCH 2/2] synccall: Add exit_sem.

The code intends for the sem_post() in line 97 (now 98) to only unblock
target threads waiting on line 29. But after the first thread is
released, the next sem_post() might also unblock a thread waiting on
line 36. That would cause the thread to return to the execution of user
code before all threads are done, leading to user code being executed in
a mixed-credentials environment.

What's more, if this happens more than once, then the mass release on
line 110 (now line 111) will cause multiple threads to execute the
callback at the same time, and the callbacks are currently not written
to cope with that situation.

Adding another semaphore allows the caller to say explicitly which
threads it wants to release.
---
 src/thread/synccall.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/thread/synccall.c b/src/thread/synccall.c
index a6b177c0..38597254 100644
--- a/src/thread/synccall.c
+++ b/src/thread/synccall.c
@@ -11,7 +11,7 @@ weak_alias(dummy_0, __tl_unlock);

 static int target_tid;
 static void (*callback)(void *), *context;
-static sem_t target_sem, caller_sem;
+static sem_t target_sem, caller_sem, exit_sem;

 static void dummy(void *p)
 {
@@ -33,7 +33,7 @@ static void handler(int sig)
 	/* Inform caller we've complered the callback and wait
 	 * for the caller to release us to return. */
 	sem_post(&caller_sem);
-	sem_wait(&target_sem);
+	sem_wait(&exit_sem);

 	/* Inform caller we are returning and state is destroyable. */
 	sem_post(&caller_sem);
@@ -62,6 +62,7 @@ void __synccall(void (*func)(void *), void *ctx)

 	sem_init(&target_sem, 0, 0);
 	sem_init(&caller_sem, 0, 0);
+	sem_init(&exit_sem, 0, 0);

 	if (!libc.threads_minus_1 || __syscall(SYS_gettid) != self->tid)
 		goto single_threaded;
@@ -107,12 +108,13 @@ single_threaded:
 	/* Only release the caught threads once all threads, including the
 	 * caller, have returned from the callback function. */
 	for (i=0; i<count; i++)
-		sem_post(&target_sem);
+		sem_post(&exit_sem);
 	for (i=0; i<count; i++)
 		sem_wait(&caller_sem);

 	sem_destroy(&caller_sem);
 	sem_destroy(&target_sem);
+	sem_destroy(&exit_sem);

 	pthread_setcancelstate(cs, 0);
 	__tl_unlock();
--
2.39.2