linux-kernel - [RFC PATCH] futex: don't retry futex_wait() with stale uaddr/val after spurious wakeup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <9179dbc3505e1de99ee36b09b0a12995239d73c3.1573079868.git.jstancek@redhat.com>
Date:   Wed,  6 Nov 2019 23:43:19 +0100
From:   Jan Stancek <jstancek@...hat.com>
To:     tglx@...utronix.de, mingo@...hat.com
Cc:     peterz@...radead.org, dvhart@...radead.org, longman@...hat.com,
        linux-kernel@...r.kernel.org, jstancek@...hat.com
Subject: [RFC PATCH] futex: don't retry futex_wait() with stale uaddr/val after spurious wakeup

Assume following scenario: process A is sleeping on futex (uaddr1)
inside futex_wait(). Process B requeues this waiter via FUTEX_CMP_REQUEUE
to uaddr2, but doesn't wake it up. Later, process A spuriously wakes up
and futex_wait() retries to queue again with same uaddr1/val1:
        if (!signal_pending(current))
                goto retry;

Problem: Userspace fails to wake process A with futex(uaddr2, FUTEX_WAKE)

Store target uaddr/val in futex_requeue() and let futex_wait()
retry after spurious wake up using stored values.

Fixes: d58e6576b0de ("futex: Handle spurious wake up")
Signed-off-by: Jan Stancek <jstancek@...hat.com>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...hat.com>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Darren Hart <dvhart@...radead.org>
Cc: Waiman Long <longman@...hat.com>
---
 kernel/futex.c | 52 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index bd18f60e4c6c..c13cfee25d35 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -237,6 +237,9 @@ struct futex_q {
 	struct rt_mutex_waiter *rt_waiter;
 	union futex_key *requeue_pi_key;
 	u32 bitset;
+
+	u32 *uaddr;
+	u32 val;
 } __randomize_layout;
 
 static const struct futex_q futex_q_init = {
@@ -1939,6 +1942,7 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 	struct futex_hash_bucket *hb1, *hb2;
 	struct futex_q *this, *next;
 	DEFINE_WAKE_Q(wake_q);
+	u32 curval, targetval;
 
 	if (nr_wake < 0 || nr_requeue < 0)
 		return -EINVAL;
@@ -2005,30 +2009,32 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 	hb_waiters_inc(hb2);
 	double_lock_hb(hb1, hb2);
 
-	if (likely(cmpval != NULL)) {
-		u32 curval;
-
+	ret = get_futex_value_locked(&targetval, uaddr2);
+	if (!ret && likely(cmpval != NULL))
 		ret = get_futex_value_locked(&curval, uaddr1);
 
-		if (unlikely(ret)) {
-			double_unlock_hb(hb1, hb2);
-			hb_waiters_dec(hb2);
+	if (unlikely(ret)) {
+		double_unlock_hb(hb1, hb2);
+		hb_waiters_dec(hb2);
 
+		ret = get_user(targetval, uaddr2);
+		if (!ret && likely(cmpval != NULL))
 			ret = get_user(curval, uaddr1);
-			if (ret)
-				goto out_put_keys;
 
-			if (!(flags & FLAGS_SHARED))
-				goto retry_private;
+		if (ret)
+			goto out_put_keys;
 
-			put_futex_key(&key2);
-			put_futex_key(&key1);
-			goto retry;
-		}
-		if (curval != *cmpval) {
-			ret = -EAGAIN;
-			goto out_unlock;
-		}
+		if (!(flags & FLAGS_SHARED))
+			goto retry_private;
+
+		put_futex_key(&key2);
+		put_futex_key(&key1);
+		goto retry;
+	}
+
+	if (likely(cmpval != NULL) && (curval != *cmpval)) {
+		ret = -EAGAIN;
+		goto out_unlock;
 	}
 
 	if (requeue_pi && (task_count - nr_wake < nr_requeue)) {
@@ -2185,6 +2191,12 @@ static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 			}
 		}
 		requeue_futex(this, hb1, hb2, &key2);
+		/*
+		 * Save target uaddr/val, in case futex_wait() wakes
+		 * up spuriously and retries to requeue.
+		 */
+		this->uaddr = uaddr2;
+		this->val = targetval;
 		drop_count++;
 	}
 
@@ -2717,6 +2729,8 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 	if (!bitset)
 		return -EINVAL;
 	q.bitset = bitset;
+	q.uaddr = uaddr;
+	q.val = val;
 
 	to = futex_setup_timer(abs_time, &timeout, flags,
 			       current->timer_slack_ns);
@@ -2725,7 +2739,7 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
 	 * Prepare to wait on uaddr. On success, holds hb lock and increments
 	 * q.key refs.
 	 */
-	ret = futex_wait_setup(uaddr, val, flags, &q, &hb);
+	ret = futex_wait_setup(q.uaddr, q.val, flags, &q, &hb);
 	if (ret)
 		goto out;
 
-- 
1.8.3.1