From: Rich Felker <dalias@libc.org>
To: jvoisin <julien.voisin@dustri.org>
Cc: musl@lists.openwall.com
Subject: Re: [musl] Protect pthreads' mutexes against use-after-destroy
Date: Sat, 20 Jan 2024 22:43:02 -0500 [thread overview]
Message-ID: <20240121034301.GZ4163@brightrain.aerifal.cx> (raw)
In-Reply-To: <20240109190726.GO4163@brightrain.aerifal.cx>
[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]
On Tue, Jan 09, 2024 at 02:07:26PM -0500, Rich Felker wrote:
> On Tue, Jan 09, 2024 at 03:37:17PM +0100, jvoisin wrote:
> > Ohai,
> >
> > as discussed on irc, Android's bionic has a check to prevent
> > use-after-destroy on phtread mutexes
> > (https://github.com/LineageOS/android_bionic/blob/e0aac7df6f58138dae903b5d456c947a3f8092ea/libc/bionic/pthread_mutex.cpp#L803),
> > and musl doesn't.
> >
> > While odds are that this is a super-duper common bug, it would still be
> > nice to have this kind of protection, since it's cheap, and would
> > prevent/make it easy to diagnose weird states.
> >
> > Is this something that should/could be implemented?
> >
> > o/
>
> I think you meant that the odds are it's not common. There's already
> enough complexity in the code paths for supporting all the different
> mutex types that my leaning would be, if we do any hardening for
> use-after-destroy, that it should probably just take the form of
> putting the object in a state that will naturally deadlock or error
> rather than adding extra checks to every path where it's used.
>
> If OTOH we do want it to actually trap in all cases where it's used
> after destroy, the simplest way to achieve that is probably to set it
> up as a non-robust non-PI recursive or errorchecking mutex with
> invalid prev/next pointers and owner of 0x3fffffff. Then the only
> place that would actually have to have an explicit trap is trylock in
> the code path:
>
> if (own == 0x3fffffff) return ENOTRECOVERABLE;
>
> where it could trap if type isn't robust. The unlock code path would
> trap on accessing invalid prev/next pointers.
Draft attached in case anyone wants to play with it. This could
probably be something we could consider to adopt.
[-- Attachment #2: trap_use_after_destroy.diff --]
[-- Type: text/plain, Size: 1829 bytes --]
diff --git a/src/thread/pthread_mutex_destroy.c b/src/thread/pthread_mutex_destroy.c
index 8d1bf77b..c56e9fbd 100644
--- a/src/thread/pthread_mutex_destroy.c
+++ b/src/thread/pthread_mutex_destroy.c
@@ -6,5 +6,13 @@ int pthread_mutex_destroy(pthread_mutex_t *mutex)
* type (tracking ownership), it might be in the pending slot of a
* robust_list; wait for quiescence. */
if (mutex->_m_type > 128) __vm_wait();
+
+ /* Setup a non-robust errorchecking mutex in ownerdead state so
+ * use after destruction can be trapped. */
+ mutex->_m_type = 3;
+ mutex->_m_prev = mutex->_m_next = 0;
+ mutex->_m_lock = 0x7fffffff;
+ mutex->_m_count = 0;
+
return 0;
}
diff --git a/src/thread/pthread_mutex_trylock.c b/src/thread/pthread_mutex_trylock.c
index a24e7c58..8d6102cb 100644
--- a/src/thread/pthread_mutex_trylock.c
+++ b/src/thread/pthread_mutex_trylock.c
@@ -21,7 +21,11 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m)
return 0;
}
}
- if (own == 0x3fffffff) return ENOTRECOVERABLE;
+ if (own == 0x3fffffff) {
+ /* Catch use-after-destroy */
+ if (!(type & 8)) a_crash();
+ return ENOTRECOVERABLE;
+ }
if (own || (old && !(type & 4))) return EBUSY;
if (type & 128) {
diff --git a/src/thread/pthread_mutex_unlock.c b/src/thread/pthread_mutex_unlock.c
index b66423e6..fe159804 100644
--- a/src/thread/pthread_mutex_unlock.c
+++ b/src/thread/pthread_mutex_unlock.c
@@ -14,8 +14,11 @@ int __pthread_mutex_unlock(pthread_mutex_t *m)
self = __pthread_self();
old = m->_m_lock;
int own = old & 0x3fffffff;
- if (own != self->tid)
+ if (own != self->tid) {
+ /* Catch use-after-destroy */
+ if (own == 0x3fffffff && !(type & 8)) a_crash();
return EPERM;
+ }
if ((type&3) == PTHREAD_MUTEX_RECURSIVE && m->_m_count)
return m->_m_count--, 0;
if ((type&4) && (old&0x40000000))
next prev parent reply other threads:[~2024-01-21 3:43 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-09 14:37 jvoisin
2024-01-09 19:07 ` Rich Felker
2024-01-09 23:27 ` enh
2024-01-10 1:58 ` Rich Felker
2024-01-12 16:53 ` enh
2024-01-10 1:55 ` Rich Felker
2024-01-10 21:24 ` Rich Felker
2024-01-21 3:43 ` Rich Felker [this message]
2024-01-21 12:06 ` julien.voisin
2024-01-21 17:03 ` Rich Felker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240121034301.GZ4163@brightrain.aerifal.cx \
--to=dalias@libc.org \
--cc=julien.voisin@dustri.org \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).