From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.1 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 92F3F23E12 for ; Sun, 21 Jan 2024 04:43:01 +0100 (CET) Received: (qmail 21594 invoked by uid 550); 21 Jan 2024 03:40:59 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 21556 invoked from network); 21 Jan 2024 03:40:58 -0000 Date: Sat, 20 Jan 2024 22:43:02 -0500 From: Rich Felker To: jvoisin Cc: musl@lists.openwall.com Message-ID: <20240121034301.GZ4163@brightrain.aerifal.cx> References: <20240109190726.GO4163@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="yFH6hwN92mUA4HK5" Content-Disposition: inline In-Reply-To: <20240109190726.GO4163@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Protect pthreads' mutexes against use-after-destroy --yFH6hwN92mUA4HK5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. --yFH6hwN92mUA4HK5 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="trap_use_after_destroy.diff" 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)) --yFH6hwN92mUA4HK5--