[PATCH v2] futex: Keep the PI owner of a private futex in the key's mm

[PATCH v2] futex: Keep the PI owner of a private futex in the key's mm

[Hyunwoo Kim]

The futex word read by the FUTEX_LOCK_PI operations can hold an arbitrary
TID. For the task looked up by that TID, attach_to_pi_owner() only checks
whether it is a kernel thread and whether it is already exiting. It does
not verify that the task belongs to the address space the futex key was
taken from.

A private futex key is the pair (mm, address), and the mm is stored as a
plain pointer without taking a reference. __attach_to_pi_owner() copies
that key into the pi_state by value and links the pi_state onto the
owner's futex.pi_state_list. The pi_state can therefore hold a key of an
mm which is not the owner's, either because the TID named a task in
another mm to begin with, or because exec_mmap() installs a new mm on the
owner after the attach. de_thread() runs before exec_mmap(), so the waiter
in the latter case is not a thread but a sibling that only shares the mm.

When the owner exits, exit_pi_state_list() pins the private hash of the
owner's mm with guard(private_hash)(current->mm), but resolves the hash
bucket from the key stored in the pi_state, which points at the waiter's
mm (M below). The stored key holds no reference on M, and
futex_hash_free() does not look at the fph references still outstanding.

  T1 (waiter, CLONE_VM sibling)     T2 (owner, mm = M -> new_mm)

  clone(CLONE_VM)   // creates M's private hash

                                 execve()
                                   exec_mmap()
                                     exec_mm_release()
                                       futex_exec_release()  // STATE_OK

  futex_lock_pi()
    attach_to_pi_owner()   // p->mm == key->private.mm
      pi_state->key = *key   // {M, addr}
      list_add(&pi_state->list, &T2->futex.pi_state_list)

                                     tsk->mm = new_mm
                                 do_exit()
                                   exit_pi_state_list()
                                     guard(private_hash)(current->mm)
                                     key = pi_state->key   // M
                                     CLASS(hbr, hbr)(&key)
                                     hb = hbr.hb           // +refcount

  do_exit()
    exit_mm()
      mmput(M)
        __mmput(M)
          futex_hash_free(M)
            kvfree(fph)

                                     spin_lock(&hb->lock)  // UAF

A private futex only has meaning inside the mm its key was taken from.
Verify in attach_to_pi_owner() that the candidate owner belongs to the mm
of the private key and return -ESRCH otherwise, as the TID is then simply
a bogus user space value. p->mm is not protected by p->pi_lock, so it is
read with READ_ONCE(), and the check is placed after the exit state check.
That check alone does not stop exec, so futex_exec_release() is split into
begin/end to keep FUTEX_STATE_EXITING set until the mm has been swapped.
An attach in that window gets -EBUSY, retries, and then fails the check
against the new mm. Fixing only one of the two leaves the same
use-after-free reachable through the other path.

Fixes: 80367ad01d93 ("futex: Add basic infrastructure for local task local hash")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
Changes in v2:
- Also keep FUTEX_STATE_EXITING set across the whole exec transition. v1 only
validated at attach time, and a successful execve() invalidates that
afterwards, so the same use-after-free stayed reachable with v1 alone.
futex_exec_release() is split into begin/end, and every exec_mmap() unwind
calls the end half.
- Rewrite the commit message to cover both paths.
- v1: https://lore.kernel.org/all/angLZrg_1yeCW0ig@v4bel/
---
 fs/exec.c             | 7 ++++++-
 include/linux/futex.h | 8 ++++++--
 kernel/fork.c         | 2 +-
 kernel/futex/core.c   | 8 +++++++-
 kernel/futex/pi.c     | 7 +++++++
 5 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index c7b8f2d6366c44..42eb98fbd12593 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -65,6 +65,7 @@
 #include <linux/io_uring.h>
 #include <linux/syscall_user_dispatch.h>
 #include <linux/coredump.h>
+#include <linux/futex.h>
 #include <linux/time_namespace.h>
 #include <linux/user_events.h>
 #include <linux/rseq.h>
@@ -857,8 +858,10 @@ static int exec_mmap(struct linux_binprm *bprm)
 	exec_mm_release(tsk, old_mm);
 
 	ret = down_write_killable(&tsk->signal->exec_update_lock);
-	if (ret)
+	if (ret) {
+		futex_exec_release_end(tsk);
 		return ret;
+	}
 
 	if (old_mm) {
 		/*
@@ -869,6 +872,7 @@ static int exec_mmap(struct linux_binprm *bprm)
 		ret = mmap_read_lock_killable(old_mm);
 		if (ret) {
 			up_write(&tsk->signal->exec_update_lock);
+			futex_exec_release_end(tsk);
 			return ret;
 		}
 	}
@@ -896,6 +900,7 @@ static int exec_mmap(struct linux_binprm *bprm)
 		local_irq_enable();
 	lru_gen_add_mm(mm);
 	task_unlock(tsk);
+	futex_exec_release_end(tsk);
 	lru_gen_use_mm(mm);
 	if (old_mm) {
 		mmap_read_unlock(old_mm);
diff --git a/include/linux/futex.h b/include/linux/futex.h
index 51f4ccdc909272..324f49493cbb43 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -72,7 +72,10 @@ static inline void futex_init_task(struct task_struct *tsk)
 
 void futex_exit_recursive(struct task_struct *tsk);
 void futex_exit_release(struct task_struct *tsk);
-void futex_exec_release(struct task_struct *tsk);
+void futex_exec_release_begin(struct task_struct *tsk)
+	__acquires(&tsk->futex.exit_mutex);
+void futex_exec_release_end(struct task_struct *tsk)
+	__releases(&tsk->futex.exit_mutex);
 
 long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 	      u32 __user *uaddr2, u32 val2, u32 val3);
@@ -90,7 +93,8 @@ static inline int futex_hash_free(struct mm_struct *mm) { return 0; }
 static inline void futex_init_task(struct task_struct *tsk) { }
 static inline void futex_exit_recursive(struct task_struct *tsk) { }
 static inline void futex_exit_release(struct task_struct *tsk) { }
-static inline void futex_exec_release(struct task_struct *tsk) { }
+static inline void futex_exec_release_begin(struct task_struct *tsk) { }
+static inline void futex_exec_release_end(struct task_struct *tsk) { }
 static inline long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 			    u32 __user *uaddr2, u32 val2, u32 val3)
 {
diff --git a/kernel/fork.c b/kernel/fork.c
index f0e2e131a9a5af..d2736b24ff7520 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1510,7 +1510,7 @@ void exit_mm_release(struct task_struct *tsk, struct mm_struct *mm)
 
 void exec_mm_release(struct task_struct *tsk, struct mm_struct *mm)
 {
-	futex_exec_release(tsk);
+	futex_exec_release_begin(tsk);
 	mm_release(tsk, mm);
 }
 
diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index 128c5752f225c2..a920dfbd0390d5 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -1539,7 +1539,8 @@ static void futex_cleanup_end(struct task_struct *tsk, int state)
 	mutex_unlock(&tsk->futex.exit_mutex);
 }
 
-void futex_exec_release(struct task_struct *tsk)
+void futex_exec_release_begin(struct task_struct *tsk)
+	__acquires(&tsk->futex.exit_mutex)
 {
 	/*
 	 * The state handling is done for consistency, but in the case of
@@ -1550,6 +1551,11 @@ void futex_exec_release(struct task_struct *tsk)
 	 */
 	futex_cleanup_begin(tsk);
 	futex_cleanup(tsk);
+}
+
+void futex_exec_release_end(struct task_struct *tsk)
+	__releases(&tsk->futex.exit_mutex)
+{
 	/*
 	 * Reset the state to FUTEX_STATE_OK. The task is alive and about
 	 * exec a new binary.
diff --git a/kernel/futex/pi.c b/kernel/futex/pi.c
index 795011ea1202f1..d97611196b34ac 100644
--- a/kernel/futex/pi.c
+++ b/kernel/futex/pi.c
@@ -465,6 +465,13 @@ static int attach_to_pi_owner(u32 __user *uaddr, u32 uval, union futex_key *key,
 		return ret;
 	}
 
+	if (IS_ENABLED(CONFIG_MMU) && futex_key_is_private(key) &&
+	    READ_ONCE(p->mm) != key->private.mm) {
+		raw_spin_unlock_irq(&p->pi_lock);
+		put_task_struct(p);
+		return -ESRCH;
+	}
+
 	__attach_to_pi_owner(p, key, ps);
 	raw_spin_unlock_irq(&p->pi_lock);
 
-- 
2.43.0