Skip site navigation (1)Skip section navigation (2) 
Date:      Wed, 4 Sep 2024 21:07:38 GMT
From:      Ed Maste <emaste@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: 094bd6281f72 - releng/14.1 - umtx: shm: Fix use-after-free due to multiple drops of the registry reference
Message-ID:  <202409042107.484L7cPg053564@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
The branch releng/14.1 has been updated by emaste:

URL: https://cgit.FreeBSD.org/src/commit/?id=094bd6281f729e51c009586136634fafa501a398

commit 094bd6281f729e51c009586136634fafa501a398
Author:     Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2024-09-04 14:38:12 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2024-09-04 20:46:54 +0000

    umtx: shm: Fix use-after-free due to multiple drops of the registry reference
    
    umtx_shm_unref_reg_locked() would unconditionally drop the "registry"
    reference, tied to USHMF_LINKED.
    
    This is not a problem for caller umtx_shm_object_terminated(), which
    operates under the 'umtx_shm_lock' lock end-to-end, but it is for
    indirect caller umtx_shm(), which drops the lock between
    umtx_shm_find_reg() and the call to umtx_shm_unref_reg(true) that
    deregisters the umtx shared region (from 'umtx_shm_registry';
    umtx_shm_find_reg() only finds registered shared mutexes).
    
    Thus, two concurrent user-space callers of _umtx_op() with UMTX_OP_SHM
    and flags UMTX_SHM_DESTROY, both progressing past umtx_shm_find_reg()
    but before umtx_shm_unref_reg(true), would then decrease twice the
    reference count for the single reference standing for the shared mutex's
    registration.
    
    Reported by:    Synacktiv
    Reviewed by:    kib
    Approved by:    emaste (mentor)
    Security:       FreeBSD-SA-24:14.umtx
    Security:       CVE-2024-43102
    Security:       CAP-01
    Sponsored by:   The Alpha-Omega Project
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D46126
    
    (cherry picked from commit 62f40433ab47ad4a9694a22a0313d57661502ca1)
    (cherry picked from commit be7dc4613909e528e8b4ea8aaa3ae3aa62bec1ed)
    
    Approved by:    so
---
 sys/kern/kern_umtx.c | 51 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/sys/kern/kern_umtx.c b/sys/kern/kern_umtx.c
index cecb4a021cfb..35fc87994c80 100644
--- a/sys/kern/kern_umtx.c
+++ b/sys/kern/kern_umtx.c
@@ -4383,39 +4383,49 @@ umtx_shm_free_reg(struct umtx_shm_reg *reg)
 }
 
 static bool
-umtx_shm_unref_reg_locked(struct umtx_shm_reg *reg, bool force)
+umtx_shm_unref_reg_locked(struct umtx_shm_reg *reg, bool linked_ref)
 {
-	bool res;
-
 	mtx_assert(&umtx_shm_lock, MA_OWNED);
 	KASSERT(reg->ushm_refcnt > 0, ("ushm_reg %p refcnt 0", reg));
-	reg->ushm_refcnt--;
-	res = reg->ushm_refcnt == 0;
-	if (res || force) {
-		if ((reg->ushm_flags & USHMF_LINKED) != 0) {
-			TAILQ_REMOVE(&umtx_shm_registry[reg->ushm_key.hash],
-			    reg, ushm_reg_link);
-			LIST_REMOVE(reg, ushm_obj_link);
-			reg->ushm_flags &= ~USHMF_LINKED;
-		}
+
+	if (linked_ref) {
+		if ((reg->ushm_flags & USHMF_LINKED) == 0)
+			/*
+			 * The reference tied to USHMF_LINKED has already been
+			 * released concurrently.
+			 */
+			return (false);
+
+		TAILQ_REMOVE(&umtx_shm_registry[reg->ushm_key.hash], reg,
+		    ushm_reg_link);
+		LIST_REMOVE(reg, ushm_obj_link);
+		reg->ushm_flags &= ~USHMF_LINKED;
 	}
-	return (res);
+
+	reg->ushm_refcnt--;
+	return (reg->ushm_refcnt == 0);
 }
 
 static void
-umtx_shm_unref_reg(struct umtx_shm_reg *reg, bool force)
+umtx_shm_unref_reg(struct umtx_shm_reg *reg, bool linked_ref)
 {
 	vm_object_t object;
 	bool dofree;
 
-	if (force) {
+	if (linked_ref) {
+		/*
+		 * Note: This may be executed multiple times on the same
+		 * shared-memory VM object in presence of concurrent callers
+		 * because 'umtx_shm_lock' is not held all along in umtx_shm()
+		 * and here.
+		 */
 		object = reg->ushm_obj->shm_object;
 		VM_OBJECT_WLOCK(object);
 		vm_object_set_flag(object, OBJ_UMTXDEAD);
 		VM_OBJECT_WUNLOCK(object);
 	}
 	mtx_lock(&umtx_shm_lock);
-	dofree = umtx_shm_unref_reg_locked(reg, force);
+	dofree = umtx_shm_unref_reg_locked(reg, linked_ref);
 	mtx_unlock(&umtx_shm_lock);
 	if (dofree)
 		umtx_shm_free_reg(reg);
@@ -4468,7 +4478,6 @@ umtx_shm_create_reg(struct thread *td, const struct umtx_key *key,
 	if (!chgumtxcnt(cred->cr_ruidinfo, 1, lim_cur(td, RLIMIT_UMTXP)))
 		return (ENOMEM);
 	reg = uma_zalloc(umtx_shm_reg_zone, M_WAITOK | M_ZERO);
-	reg->ushm_refcnt = 1;
 	bcopy(key, &reg->ushm_key, sizeof(*key));
 	reg->ushm_obj = shm_alloc(td->td_ucred, O_RDWR, false);
 	reg->ushm_cred = crhold(cred);
@@ -4485,11 +4494,17 @@ umtx_shm_create_reg(struct thread *td, const struct umtx_key *key,
 		*res = reg1;
 		return (0);
 	}
-	reg->ushm_refcnt++;
 	TAILQ_INSERT_TAIL(&umtx_shm_registry[key->hash], reg, ushm_reg_link);
 	LIST_INSERT_HEAD(USHM_OBJ_UMTX(key->info.shared.object), reg,
 	    ushm_obj_link);
 	reg->ushm_flags = USHMF_LINKED;
+	/*
+	 * This is one reference for the registry and the list of shared
+	 * mutexes referenced by the VM object containing the lock pointer, and
+	 * another for the caller, which it will free after use.  So, one of
+	 * these is tied to the presence of USHMF_LINKED.
+	 */
+	reg->ushm_refcnt = 2;
 	mtx_unlock(&umtx_shm_lock);
 	*res = reg;
 	return (0);

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202409042107.484L7cPg053564>