From 517d5c245c9805b56f73c7fa0e23e8853fe22da6 Mon Sep 17 00:00:00 2001 From: Artem Savkov Date: Fri, 21 May 2021 14:20:32 +0200 Subject: [RHEL7.9 KPATCH] CVE-2021-3347 Use after free via PI futex state Kernels: 3.10.0-1160.el7 3.10.0-1160.2.1.el7 3.10.0-1160.2.2.el7 3.10.0-1160.6.1.el7 3.10.0-1160.11.1.el7 3.10.0-1160.15.2.el7 3.10.0-1160.21.1.el7 3.10.0-1160.24.1.el7 3.10.0-1160.25.1.el7 Changes since last build: [x86_64]: futex.o: changed function: do_futex futex.o: changed function: fixup_owner futex.o: changed function: fixup_pi_state_owner.isra.16 futex.o: changed function: free_pi_state futex.o: changed function: futex_lock_pi.isra.20 futex.o: changed function: futex_wait_requeue_pi.constprop.22 futex.o: new function: pi_state_update_owner [ppc64le]: futex.o: changed function: do_futex futex.o: changed function: fixup_owner futex.o: changed function: fixup_pi_state_owner.isra.9 futex.o: changed function: free_pi_state futex.o: changed function: futex_lock_pi.isra.16 futex.o: changed function: futex_wait_requeue_pi.constprop.17 futex.o: changed function: unqueue_me_pi futex.o: new function: pi_state_update_owner --------------------------- Modifications: added -fno-optimize-sibling-calls to fixup_owner() commit d2fb2a9cf682bdba4b66103fb079c13a04039430 Author: Donghai Qiao Date: Thu May 20 16:35:49 2021 -0400 futex: Handle faults correctly for PI futexes Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1935108 Upstream status: 34b1a1ce1458f50ef27c54e28eb9b1947012907a CVE: CVE-2021-3347 Conflicts: The original patch is intent to make the state of rtmutex and pi_state consistent if the kernel is unable to update the user space futex word, rather than unlocking the rtmutex and leaving pi_state out of synched. As a result, this original fix removed part of the code which was introduced by 16ffa12d7 ("futex: Pull rt_mutex_futex_unlock() out from under hb->lock") to the functions futex_lock_pi() and futex_wait_requeue_pi() to avoid the inconsistency. So the conflicts are related to the following two commits, though git blame displayed a much longer list which shows the chain of dependency in the history. 16ffa12d7425 ("futex: Pull rt_mutex_futex_unlock() out from under hb->lock") c236c8e95a3d ("futex: Fix potential use-after-free in FUTEX_REQUEUE_PI") commit 34b1a1ce1458f50ef27c54e28eb9b1947012907a Author: Thomas Gleixner Date: Mon, 18 Jan 2021 19:01:21 +0100 futex: Handle faults correctly for PI futexes fixup_pi_state_owner() tries to ensure that the state of the rtmutex, pi_state and the user space value related to the PI futex are consistent before returning to user space. In case that the user space value update faults and the fault cannot be resolved by faulting the page in via fault_in_user_writeable() the function returns with -EFAULT and leaves the rtmutex and pi_state owner state inconsistent. A subsequent futex_unlock_pi() operates on the inconsistent pi_state and releases the rtmutex despite not owning it which can corrupt the RB tree of the rtmutex and cause a subsequent kernel stack use after free. It was suggested to loop forever in fixup_pi_state_owner() if the fault cannot be resolved, but that results in runaway tasks which is especially undesired when the problem happens due to a programming error and not due to malice. As the user space value cannot be fixed up, the proper solution is to make the rtmutex and the pi_state consistent so both have the same owner. This leaves the user space value out of sync. Any subsequent operation on the futex will fail because the 10th rule of PI futexes (pi_state owner and user space value are consistent) has been violated. As a consequence this removes the inept attempts of 'fixing' the situation in case that the current task owns the rtmutex when returning with an unresolvable fault by unlocking the rtmutex which left pi_state::owner and rtmutex::owner out of sync in a different and only slightly less dangerous way. Fixes: 1b7558e457ed ("futexes: fix fault handling in futex_lock_pi") Reported-by: gzobqq@gmail.com Signed-off-by: Thomas Gleixner Acked-by: Peter Zijlstra (Intel) Cc: stable@vger.kernel.org Signed-off-by: Donghai Qiao commit 25077b49b47c1cdf224b54c837172ff820e8be88 Author: Donghai Qiao Date: Thu May 20 16:30:16 2021 -0400 futex: Provide and use pi_state_update_owner() Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1935108 Upstream status: c5cade200ab9a2a3be9e7f32a752c8d86b502ec7 CVE: CVE-2021-3347 Conflicts: Updating the owner of pi_state requires that we remove the pi_state structure from the old owner's pi_state_list then add it to the new owner's pi_state_list. Because this action takes place in multiple occassions in the current upstream futex.c, so the similar code is duplicated in all these places. The purpose of this patch is to eliminate these code duplications with a new routine pi_state_update_owner(). The conflicts in 7.9.z are caused by the differences in places where updating owner takes place. After sorting out the details, the relevant commit IDs as below : 734009e96d19 ("futex: Change locking rules") b4abf91047cf ("rtmutex: Make wait_lock irq safe") commit c5cade200ab9a2a3be9e7f32a752c8d86b502ec7 Author: Thomas Gleixner Date: Tue, 19 Jan 2021 15:21:35 +0100 futex: Provide and use pi_state_update_owner() Updating pi_state::owner is done at several places with the same code. Provide a function for it and use that at the obvious places. This is also a preparation for a bug fix to avoid yet another copy of the same code or alternatively introducing a completely unpenetratable mess of gotos. Originally-by: Peter Zijlstra Signed-off-by: Thomas Gleixner Acked-by: Peter Zijlstra (Intel) Cc: stable@vger.kernel.org Signed-off-by: Donghai Qiao commit 69414a50f8bad2063b89981110fb374733209d9d Author: Donghai Qiao Date: Wed May 19 14:24:04 2021 -0400 futex: Replace pointless printk in fixup_owner() Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1935108 Upstream status: 04b79c55201f02ffd675e1231d731365e335c307 CVE: CVE-2021-3347 commit 04b79c55201f02ffd675e1231d731365e335c307 Author: Thomas Gleixner Date: Tue, 19 Jan 2021 16:06:10 +0100 futex: Replace pointless printk in fixup_owner() If that unexpected case of inconsistent arguments ever happens then the futex state is left completely inconsistent and the printk is not really helpful. Replace it with a warning and make the state consistent. Signed-off-by: Thomas Gleixner Acked-by: Peter Zijlstra (Intel) Cc: stable@vger.kernel.org Signed-off-by: Donghai Qiao commit 7e96fb06469c95628039ead2591f82e88af5da10 Author: Donghai Qiao Date: Wed May 19 14:19:05 2021 -0400 futex: Ensure the correct return value from futex_lock_pi() Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1935108 Upstream status: 12bb3f7f1b03d5913b3f9d4236a488aa7774dfe9 CVE: CVE-2021-3347 Conflicts: This original upstream patch relies heavily on c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex") which is one of the upstream commits listed below. But the backport for c1e2f0eaf015 requires we resolve very complex chain of dependencies across multiple critical kernel source files therefore the risk is considered too high for 7.9.z. Instead of pulling together tons of the relevant commits in to 7.9.z, we just want to take a light risk approach by digesting the fix 12bb3f7f1b03 ("futex: Ensure the correct return value from futex_lock_pi()") for 7.9.z. All we need to do is to make the changed functions fixup_owner() and fixup_pi_state_owner() of 7.9.z return the required values as this upstream fix suggests in every circumstance. This way, we can cleanly cut this CVE patch set with merely four patches, without having to backport tons of patches in the chain of dependency. Besides, an extra change made to fixup_owner() (see HUNK -2063,13 +2062,11 in this backport patch) is to eliminate a mistake made by upstream, where the specification of a local variable "ret" was removed from that function, but there was still a dereference to "ret" as shown by that HUNK. 16ffa12d7425 ("futex: Pull rt_mutex_futex_unlock() out from under hb->lock") c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex") 734009e96d19 ("futex: Change locking rules") d7c5ed73b19c ("futex: Remove needless goto's") 6b4f4bc9cb22 ("locking/futex: Allow low-level atomic operations to return -EAGAIN") commit 12bb3f7f1b03d5913b3f9d4236a488aa7774dfe9 Author: Thomas Gleixner Date: Wed, 20 Jan 2021 16:00:24 +0100 futex: Ensure the correct return value from futex_lock_pi() In case that futex_lock_pi() was aborted by a signal or a timeout and the task returned without acquiring the rtmutex, but is the designated owner of the futex due to a concurrent futex_unlock_pi() fixup_owner() is invoked to establish consistent state. In that case it invokes fixup_pi_state_owner() which in turn tries to acquire the rtmutex again. If that succeeds then it does not propagate this success to fixup_owner() and futex_lock_pi() returns -EINTR or -ETIMEOUT despite having the futex locked. Return success from fixup_pi_state_owner() in all cases where the current task owns the rtmutex and therefore the futex and propagate it correctly through fixup_owner(). Fixup the other callsite which does not expect a positive return value. Fixes: c1e2f0eaf015 ("futex: Avoid violating the 10th rule of futex") Signed-off-by: Thomas Gleixner Acked-by: Peter Zijlstra (Intel) Cc: stable@vger.kernel.org Signed-off-by: Donghai Qiao Signed-off-by: Artem Savkov Acked-by: Joe Lawrence Acked-by: Yannick Cote --- kernel/futex.c | 123 +++++++++++++++++++++++++------------------------ 1 file changed, 63 insertions(+), 60 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index 877831775d7aa..8ec57c357ca58 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -640,6 +640,29 @@ static struct futex_pi_state * alloc_pi_state(void) return pi_state; } +static void pi_state_update_owner(struct futex_pi_state *pi_state, + struct task_struct *new_owner) +{ + struct task_struct *old_owner = pi_state->owner; + + lockdep_assert_held(&pi_state->pi_mutex.wait_lock); + + if (old_owner) { + raw_spin_lock_irq(&old_owner->pi_lock); + WARN_ON(list_empty(&pi_state->list)); + list_del_init(&pi_state->list); + raw_spin_unlock_irq(&old_owner->pi_lock); + } + + if (new_owner) { + raw_spin_lock_irq(&new_owner->pi_lock); + WARN_ON(!list_empty(&pi_state->list)); + list_add(&pi_state->list, &new_owner->pi_state_list); + pi_state->owner = new_owner; + raw_spin_unlock_irq(&new_owner->pi_lock); + } +} + static void free_pi_state(struct futex_pi_state *pi_state) { if (!atomic_dec_and_test(&pi_state->refcount)) @@ -650,10 +673,7 @@ static void free_pi_state(struct futex_pi_state *pi_state) * and has cleaned up the pi_state already */ if (pi_state->owner) { - raw_spin_lock_irq(&pi_state->owner->pi_lock); - list_del_init(&pi_state->list); - raw_spin_unlock_irq(&pi_state->owner->pi_lock); - + pi_state_update_owner(pi_state, NULL); rt_mutex_proxy_unlock(&pi_state->pi_mutex, pi_state->owner); } @@ -791,7 +811,8 @@ void exit_pi_state_list(struct task_struct *curr) * FUTEX_OWNER_DIED bit. See [4] * * [10] There is no transient state which leaves owner and user space - * TID out of sync. + * TID out of sync. Except one error case where the kernel is denied + * write access to the user address, see fixup_pi_state_owner(). */ static int lookup_pi_state(u32 uval, struct futex_hash_bucket *hb, @@ -1168,16 +1189,7 @@ static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this) return ret; } - raw_spin_lock_irq(&pi_state->owner->pi_lock); - WARN_ON(list_empty(&pi_state->list)); - list_del_init(&pi_state->list); - raw_spin_unlock_irq(&pi_state->owner->pi_lock); - - raw_spin_lock_irq(&new_owner->pi_lock); - WARN_ON(!list_empty(&pi_state->list)); - list_add(&pi_state->list, &new_owner->pi_state_list); - pi_state->owner = new_owner; - raw_spin_unlock_irq(&new_owner->pi_lock); + pi_state_update_owner(pi_state, new_owner); raw_spin_unlock(&pi_state->pi_mutex.wait_lock); rt_mutex_unlock(&pi_state->pi_mutex); @@ -1953,20 +1965,9 @@ retry: * We fixed up user space. Now we need to fix the pi_state * itself. */ - if (pi_state->owner != NULL) { - raw_spin_lock_irq(&pi_state->owner->pi_lock); - WARN_ON(list_empty(&pi_state->list)); - list_del_init(&pi_state->list); - raw_spin_unlock_irq(&pi_state->owner->pi_lock); - } + pi_state_update_owner(pi_state, newowner); - pi_state->owner = newowner; - - raw_spin_lock_irq(&newowner->pi_lock); - WARN_ON(!list_empty(&pi_state->list)); - list_add(&pi_state->list, &newowner->pi_state_list); - raw_spin_unlock_irq(&newowner->pi_lock); - return 0; + return newowner == current; /* * To handle the page fault we need to drop the hash bucket @@ -1989,10 +1990,26 @@ handle_fault: * Check if someone else fixed it for us: */ if (pi_state->owner != oldowner) - return 0; + return newowner == current; + + if (ret) { + /* + * fault_in_user_writeable() failed so user state is immutable. At + * best we can make the kernel state consistent but user state will + * be most likely hosed and any subsequent unlock operation will be + * rejected due to PI futex rule [10]. + * + * Ensure that the rtmutex owner is also the pi_state owner despite + * the user space value claiming something different. There is no + * point in unlocking the rtmutex if current is the owner as it + * would need to wait until the next waiter has taken the rtmutex + * to guarantee consistent state. Keep it simple. Userspace asked + * for this wreckaged state. + */ + pi_state_update_owner(pi_state, rt_mutex_owner(&pi_state->pi_mutex)); - if (ret) return ret; + } goto retry; } @@ -2014,10 +2031,10 @@ static long futex_wait_restart(struct restart_block *restart); * 0 - success, lock not taken; * <0 - on error (-EFAULT) */ +__attribute__((optimize("-fno-optimize-sibling-calls"))) static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked) { struct task_struct *owner; - int ret = 0; if (locked) { /* @@ -2025,8 +2042,8 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked) * did a lock-steal - fix up the PI-state in that case: */ if (q->pi_state->owner != current) - ret = fixup_pi_state_owner(uaddr, q, current); - goto out; + return fixup_pi_state_owner(uaddr, q, current); + return 1; } /* @@ -2040,8 +2057,7 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked) * rt_mutex waiters list. */ if (rt_mutex_trylock(&q->pi_state->pi_mutex)) { - locked = 1; - goto out; + return 1; } /* @@ -2054,22 +2070,18 @@ static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked) if (!owner) owner = rt_mutex_next_owner(&q->pi_state->pi_mutex); raw_spin_unlock(&q->pi_state->pi_mutex.wait_lock); - ret = fixup_pi_state_owner(uaddr, q, owner); - goto out; + + return fixup_pi_state_owner(uaddr, q, owner); } /* * Paranoia check. If we did not take the lock, then we should not be - * the owner of the rt_mutex. + * the owner of the rt_mutex. Warn and establish consistent state. */ - if (rt_mutex_owner(&q->pi_state->pi_mutex) == current) - printk(KERN_ERR "fixup_owner: ret = %d pi-mutex: %p " - "pi-state %p\n", ret, - q->pi_state->pi_mutex.owner, - q->pi_state->owner); + if (WARN_ON_ONCE(rt_mutex_owner(&q->pi_state->pi_mutex) == current)) + return fixup_pi_state_owner(uaddr, q, current); -out: - return ret ? ret : locked; + return 0; } /** @@ -2363,13 +2375,6 @@ retry_private: if (res) ret = (res < 0) ? res : 0; - /* - * If fixup_owner() faulted and was unable to handle the fault, unlock - * it and return the fault to userspace. - */ - if (ret && (rt_mutex_owner(&q.pi_state->pi_mutex) == current)) - rt_mutex_unlock(&q.pi_state->pi_mutex); - /* Unqueue and drop the lock */ unqueue_me_pi(&q); @@ -2666,6 +2671,11 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, spin_lock(q.lock_ptr); ret = fixup_pi_state_owner(uaddr2, &q, current); spin_unlock(q.lock_ptr); + /* + * Adjust the return value. It's either -EFAULT or + * success (1) but the caller expects 0 for success. + */ + ret = ret < 0 ? ret : 0; } } else { /* @@ -2695,14 +2705,7 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, unqueue_me_pi(&q); } - /* - * If fixup_pi_state_owner() faulted and was unable to handle the - * fault, unlock the rt_mutex and return the fault to userspace. - */ - if (ret == -EFAULT) { - if (pi_mutex && rt_mutex_owner(pi_mutex) == current) - rt_mutex_unlock(pi_mutex); - } else if (ret == -EINTR) { + if (ret == -EINTR) { /* * We've already been requeued, but cannot restart by calling * futex_lock_pi() directly. We could restart this syscall, but -- 2.26.3