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 --- 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 #include #include +#include #include #include #include @@ -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