mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] pthread shouldn't ignore errors from syscall futex()
@ 2020-05-20 12:31 Konstantin Khlebnikov
  2020-05-20 16:05 ` Rich Felker
  0 siblings, 1 reply; 4+ messages in thread
From: Konstantin Khlebnikov @ 2020-05-20 12:31 UTC (permalink / raw)
  To: musl

Userspace implementations of mutexes (including glibc) in some cases
retries operation without checking error code from syscall futex.

Example which loops inside second call rather than hung (or die) peacefully:

#include <stdlib.h>
#include <pthread.h>

int main(int argc, char **argv)
{
	char buf[sizeof(pthread_mutex_t) + 1];
	pthread_mutex_t *mutex = (pthread_mutex_t *)(buf + 1);

	pthread_mutex_init(mutex, NULL);
	pthread_mutex_lock(mutex);
	pthread_mutex_lock(mutex);
}

Thread in lkml:
https://lore.kernel.org/lkml/158955700764.647498.18025770126733698386.stgit@buzz/T/

Related bug in glibc:
https://sourceware.org/bugzilla/show_bug.cgi?id=25997

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [musl] pthread shouldn't ignore errors from syscall futex()
  2020-05-20 12:31 [musl] pthread shouldn't ignore errors from syscall futex() Konstantin Khlebnikov
@ 2020-05-20 16:05 ` Rich Felker
  2020-05-20 17:38   ` Konstantin Khlebnikov
  0 siblings, 1 reply; 4+ messages in thread
From: Rich Felker @ 2020-05-20 16:05 UTC (permalink / raw)
  To: musl; +Cc: Konstantin Khlebnikov

On Wed, May 20, 2020 at 03:31:46PM +0300, Konstantin Khlebnikov wrote:
> Userspace implementations of mutexes (including glibc) in some cases
> retries operation without checking error code from syscall futex.
> 
> Example which loops inside second call rather than hung (or die) peacefully:
> 
> #include <stdlib.h>
> #include <pthread.h>
> 
> int main(int argc, char **argv)
> {
> 	char buf[sizeof(pthread_mutex_t) + 1];
> 	pthread_mutex_t *mutex = (pthread_mutex_t *)(buf + 1);
> 
> 	pthread_mutex_init(mutex, NULL);
> 	pthread_mutex_lock(mutex);
> 	pthread_mutex_lock(mutex);
> }
> 
> Thread in lkml:
> https://lore.kernel.org/lkml/158955700764.647498.18025770126733698386.stgit@buzz/T/
> 
> Related bug in glibc:
> https://sourceware.org/bugzilla/show_bug.cgi?id=25997

In general, this behavior is intentional. If running on a system where
futexx is broken (incomplete implementation of Linux syscall API,
Linux built with flags that break futex which is possible on some
archs, etc.), or if the kernel cannot perform the wait because of an
OOM condition in the kernel (Linux is *not* written to be resilent
against OOM and it shows), the behavior degrades to spinlocks rather
than crashing. Aborting the application because of OOM conditions in
the kernel is simply not acceptable.

It would be possible to try to distinguish the causes of futex failure
and handle the unaligned case specially, but this would put more code
in hot paths, impacting size and possibly performance in valid
programs for the sake of catching a non-security bug in invalid ones.
This does not seem like a useful tradeoff.

Assuming the buggy program actually calls pthread_mutex_init rather
than just using an uninitialized/zero-initialized mutex object at
misaligned address, pthread_mutex_init (and likewise other pthread
object init functions) could possibly trap on the error (with no
syscall, just looking for a misaligned address mod _Alignof() the
object type) to catch it. I'm not sure if this is worthwhile though
since, while being UB, it doesn't seem to be UB with any security
impact.

Rich

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [musl] pthread shouldn't ignore errors from syscall futex()
  2020-05-20 16:05 ` Rich Felker
@ 2020-05-20 17:38   ` Konstantin Khlebnikov
  2020-05-20 17:49     ` Rich Felker
  0 siblings, 1 reply; 4+ messages in thread
From: Konstantin Khlebnikov @ 2020-05-20 17:38 UTC (permalink / raw)
  To: Rich Felker, musl

On 20/05/2020 19.05, Rich Felker wrote:
> On Wed, May 20, 2020 at 03:31:46PM +0300, Konstantin Khlebnikov wrote:
>> Userspace implementations of mutexes (including glibc) in some cases
>> retries operation without checking error code from syscall futex.
>>
>> Example which loops inside second call rather than hung (or die) peacefully:
>>
>> #include <stdlib.h>
>> #include <pthread.h>
>>
>> int main(int argc, char **argv)
>> {
>> 	char buf[sizeof(pthread_mutex_t) + 1];
>> 	pthread_mutex_t *mutex = (pthread_mutex_t *)(buf + 1);
>>
>> 	pthread_mutex_init(mutex, NULL);
>> 	pthread_mutex_lock(mutex);
>> 	pthread_mutex_lock(mutex);
>> }
>>
>> Thread in lkml:
>> https://lore.kernel.org/lkml/158955700764.647498.18025770126733698386.stgit@buzz/T/
>>
>> Related bug in glibc:
>> https://sourceware.org/bugzilla/show_bug.cgi?id=25997
> 
> In general, this behavior is intentional. If running on a system where
> futexx is broken (incomplete implementation of Linux syscall API,
> Linux built with flags that break futex which is possible on some
> archs, etc.), or if the kernel cannot perform the wait because of an
> OOM condition in the kernel (Linux is *not* written to be resilent
> against OOM and it shows), the behavior degrades to spinlocks rather
> than crashing. Aborting the application because of OOM conditions in
> the kernel is simply not acceptable.

Yes, OOM condition in cgroup before linux 4.19 definitely could lead to
returning EFAULT by almost any syscall. This is worth to document in
futex manpage.

But EINVAL from futex() always meant arguments were wrong.

Ignoring unknown errors feels wrong anyway. That just hides bugs.
And provokes appearing these incomplete/buggy implementations of futex.

Also degrading silently to spin-locks isn't very safe.
Not all schedulers guarantee progress if waiter spins.
At least add some delay or yield into that fallback waiting loop.

> 
> It would be possible to try to distinguish the causes of futex failure
> and handle the unaligned case specially, but this would put more code
> in hot paths, impacting size and possibly performance in valid
> programs for the sake of catching a non-security bug in invalid ones.
> This does not seem like a useful tradeoff.

I've proposed to send SIGBUS from syscall when futex address is unligned.
(In LKML thread, see link above)

> 
> Assuming the buggy program actually calls pthread_mutex_init rather
> than just using an uninitialized/zero-initialized mutex object at
> misaligned address, pthread_mutex_init (and likewise other pthread
> object init functions) could possibly trap on the error (with no
> syscall, just looking for a misaligned address mod _Alignof() the
> object type) to catch it. I'm not sure if this is worthwhile though
> since, while being UB, it doesn't seem to be UB with any security
> impact.

Yeah, I'm worried more about debugability and CO2 emission =)

> 
> Rich
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [musl] pthread shouldn't ignore errors from syscall futex()
  2020-05-20 17:38   ` Konstantin Khlebnikov
@ 2020-05-20 17:49     ` Rich Felker
  0 siblings, 0 replies; 4+ messages in thread
From: Rich Felker @ 2020-05-20 17:49 UTC (permalink / raw)
  To: Konstantin Khlebnikov; +Cc: musl

On Wed, May 20, 2020 at 08:38:35PM +0300, Konstantin Khlebnikov wrote:
> On 20/05/2020 19.05, Rich Felker wrote:
> >On Wed, May 20, 2020 at 03:31:46PM +0300, Konstantin Khlebnikov wrote:
> >>Userspace implementations of mutexes (including glibc) in some cases
> >>retries operation without checking error code from syscall futex.
> >>
> >>Example which loops inside second call rather than hung (or die) peacefully:
> >>
> >>#include <stdlib.h>
> >>#include <pthread.h>
> >>
> >>int main(int argc, char **argv)
> >>{
> >>	char buf[sizeof(pthread_mutex_t) + 1];
> >>	pthread_mutex_t *mutex = (pthread_mutex_t *)(buf + 1);
> >>
> >>	pthread_mutex_init(mutex, NULL);
> >>	pthread_mutex_lock(mutex);
> >>	pthread_mutex_lock(mutex);
> >>}
> >>
> >>Thread in lkml:
> >>https://lore.kernel.org/lkml/158955700764.647498.18025770126733698386.stgit@buzz/T/
> >>
> >>Related bug in glibc:
> >>https://sourceware.org/bugzilla/show_bug.cgi?id=25997
> >
> >In general, this behavior is intentional. If running on a system where
> >futexx is broken (incomplete implementation of Linux syscall API,
> >Linux built with flags that break futex which is possible on some
> >archs, etc.), or if the kernel cannot perform the wait because of an
> >OOM condition in the kernel (Linux is *not* written to be resilent
> >against OOM and it shows), the behavior degrades to spinlocks rather
> >than crashing. Aborting the application because of OOM conditions in
> >the kernel is simply not acceptable.
> 
> Yes, OOM condition in cgroup before linux 4.19 definitely could lead to
> returning EFAULT by almost any syscall. This is worth to document in
> futex manpage.
> 
> But EINVAL from futex() always meant arguments were wrong.
> 
> Ignoring unknown errors feels wrong anyway. That just hides bugs.
> And provokes appearing these incomplete/buggy implementations of futex.

Ignoring errors is never wrong unless the error can happen in a
correct program or ignoring it in an incorrect program produces a
vulnerability that affects the user's safety.

> Also degrading silently to spin-locks isn't very safe.
> Not all schedulers guarantee progress if waiter spins.
> At least add some delay or yield into that fallback waiting loop.

We are not going to add costs to valid programs to make up for bad
behavior by buggy programs unless it's mitigating a security issue
potentially leading to code execution, information disclosure, loss of
data integrity, or something similarly severe. "Box bogged down at
100% cpu because you ran a buggy program" is not something that merits
libc trying to mitigate it.

> >It would be possible to try to distinguish the causes of futex failure
> >and handle the unaligned case specially, but this would put more code
> >in hot paths, impacting size and possibly performance in valid
> >programs for the sake of catching a non-security bug in invalid ones.
> >This does not seem like a useful tradeoff.
> 
> I've proposed to send SIGBUS from syscall when futex address is unligned.
> (In LKML thread, see link above)

And they rightly rejected it. Arguably every EFAULT should also
produce SIGBUS or SIGSEGV, but they don't, because the policy has
always been to report erroneous userspace addresses to userspace
rather than crashing. Changing this would be a major change in kernel
policy, and a break in stability policy. For example there may even be
applications calling futex on a misaligned address to *test whether*
it's supported usage, relying on the existing contract to report it.

> >Assuming the buggy program actually calls pthread_mutex_init rather
> >than just using an uninitialized/zero-initialized mutex object at
> >misaligned address, pthread_mutex_init (and likewise other pthread
> >object init functions) could possibly trap on the error (with no
> >syscall, just looking for a misaligned address mod _Alignof() the
> >object type) to catch it. I'm not sure if this is worthwhile though
> >since, while being UB, it doesn't seem to be UB with any security
> >impact.
> 
> Yeah, I'm worried more about debugability and CO2 emission =)

Then proposing this for the init functions is probably a good idea. In
musl we generally like to trap on UB, but only when it either happens
naturally (like null arg dereference) or can be done at low cost,
ideally not in a hot path. So this seems like a reasonable solution to
achieve what you want.

Rich

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-05-20 17:49 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 12:31 [musl] pthread shouldn't ignore errors from syscall futex() Konstantin Khlebnikov
2020-05-20 16:05 ` Rich Felker
2020-05-20 17:38   ` Konstantin Khlebnikov
2020-05-20 17:49     ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).