futex: Fix potential use-after-free in FUTEX_REQUEUE_PI

While working on the futex code, I stumbled over this potential use-after-free scenario.

pi_mutex is a pointer into pi_state, which we drop the reference on in unqueue_me_pi(). So any access to that pointer after that is bad.

Since other sites already do rt_mutex_unlock() with hb->lock held, see for example futex_lock_pi(), simply move the unlock before unqueue_me_pi().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/futex.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

kernel/futex.c +0 -1

index 2c4be467fecd..d5a81339209f 100644
@@ -2813,7 +2813,6 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 {
 	struct hrtimer_sleeper timeout, *to = NULL;
 	struct rt_mutex_waiter rt_waiter;
-	struct rt_mutex *pi_mutex = NULL;
 	struct futex_hash_bucket *hb;
 	union futex_key key2 = FUTEX_KEY_INIT;
 	struct futex_q q = futex_q_init;

Quoted 25 lines · Peter Zijlstra

While working on the futex code, I stumbled over this potential use-after-free scenario.

pi_mutex is a pointer into pi_state, which we drop the reference on in unqueue_me_pi(). So any access to that pointer after that is bad.

Since other sites already do rt_mutex_unlock() with hb->lock held, see for example futex_lock_pi(), simply move the unlock before unqueue_me_pi().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/futex.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

kernel/futex.c +0 -1

index 2c4be467fecd..d5a81339209f 100644
@@ -2813,7 +2813,6 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 {
 	struct hrtimer_sleeper timeout, *to = NULL;
 	struct rt_mutex_waiter rt_waiter;
-	struct rt_mutex *pi_mutex = NULL;

kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2904

Quoted 5 lines

kernel/futex.c +0 -0

 	struct futex_hash_bucket *hb;
 	union futex_key key2 = FUTEX_KEY_INIT;
 	struct futex_q q = futex_q_init;

@@ -2905,6 +2904,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 			spin_unlock(q.lock_ptr);

In this path the fixup can return -EFAIL as well, so it should drop rtmutex too if it owns it. We should move the rtmutex drop into the fixup functions...

Quoted 27 lines · Thomas Gleixner

Quoted 25 lines · Peter Zijlstra

While working on the futex code, I stumbled over this potential use-after-free scenario.

pi_mutex is a pointer into pi_state, which we drop the reference on in unqueue_me_pi(). So any access to that pointer after that is bad.

Since other sites already do rt_mutex_unlock() with hb->lock held, see for example futex_lock_pi(), simply move the unlock before unqueue_me_pi().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/futex.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

kernel/futex.c +0 -1

index 2c4be467fecd..d5a81339209f 100644
@@ -2813,7 +2813,6 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 {
 	struct hrtimer_sleeper timeout, *to = NULL;
 	struct rt_mutex_waiter rt_waiter;
-	struct rt_mutex *pi_mutex = NULL;

Quoted 8 lines

kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2904

Quoted 5 lines

kernel/futex.c +0 -0

 	struct futex_hash_bucket *hb;
 	union futex_key key2 = FUTEX_KEY_INIT;
 	struct futex_q q = futex_q_init;

@@ -2905,6 +2904,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 			spin_unlock(q.lock_ptr);

In this path the fixup can return -EFAIL as well, so it should drop rtmutex too if it owns it. We should move the rtmutex drop into the fixup functions...

Urgh, so would really like to avoid doing that, I'll have to instantly drag it back out again :/

Quoted 5 lines · Peter Zijlstra

Quoted 2 lines

In this path the fixup can return -EFAIL as well, so it should drop rtmutex too if it owns it. We should move the rtmutex drop into the fixup functions...

Urgh, so would really like to avoid doing that, I'll have to instantly drag it back out again :/

Why would you have to drag it back out again? Something else you're working on?

Quoted 27 lines · Thomas Gleixner

Quoted 25 lines · Peter Zijlstra

While working on the futex code, I stumbled over this potential use-after-free scenario.

pi_mutex is a pointer into pi_state, which we drop the reference on in unqueue_me_pi(). So any access to that pointer after that is bad.

Since other sites already do rt_mutex_unlock() with hb->lock held, see for example futex_lock_pi(), simply move the unlock before unqueue_me_pi().

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/futex.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

kernel/futex.c +0 -1

index 2c4be467fecd..d5a81339209f 100644
@@ -2813,7 +2813,6 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 {
 	struct hrtimer_sleeper timeout, *to = NULL;
 	struct rt_mutex_waiter rt_waiter;
-	struct rt_mutex *pi_mutex = NULL;

Quoted 8 lines

kernel/futex.c static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags, · line 2904

Quoted 5 lines

kernel/futex.c +0 -0

 	struct futex_hash_bucket *hb;
 	union futex_key key2 = FUTEX_KEY_INIT;
 	struct futex_q q = futex_q_init;

@@ -2905,6 +2904,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 			spin_unlock(q.lock_ptr);

In this path the fixup can return -EFAIL as well, so it should drop rtmutex too if it owns it. We should move the rtmutex drop into the fixup functions...

Urgh, so would really like to avoid doing that, I'll have to instantly drag it back out again :/

Also, the fixup_owner() fail in futex_lock_pi() will unlock the rt_mutex on _any_ fail, not only -EFAULT, should we not do the same?

---
Subject: futex: Fix potential use-after-free in FUTEX_REQUEUE_PI
From: Peter Zijlstra <[email protected]>
Date: Thu, 24 Nov 2016 15:42:35 +0100

While working on the futex code, I stumbled over this potential use-after-free scenario.

pi_mutex is a pointer into pi_state, which we drop the reference on in unqueue_me_pi(). So any access to that pointer after that is bad.

Since other sites already do rt_mutex_unlock() with hb->lock held, see for example futex_lock_pi(), simply move the unlock before unqueue_me_pi().

Cc Ingo Molnar <[email protected]>

Cc [email protected]

Cc Thomas Gleixner <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/futex.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

kernel/futex.c +4 -1

@@ -2813,7 +2813,6 @@ static int futex_wait_requeue_pi(u32 __u
 {
 	struct hrtimer_sleeper timeout, *to = NULL;
 	struct rt_mutex_waiter rt_waiter;
-	struct rt_mutex *pi_mutex = NULL;
 	struct futex_hash_bucket *hb;
 	union futex_key key2 = FUTEX_KEY_INIT;
 	struct futex_q q = futex_q_init;

@@ -2897,6 +2896,8 @@ static int futex_wait_requeue_pi(u32 __u
 		if (q.pi_state && (q.pi_state->owner != current)) {
 			spin_lock(q.lock_ptr);
 			ret = fixup_pi_state_owner(uaddr2, &q, current);
+			if (ret && rt_mutex_owner(&q.pi_state->pi_mutex) == current)
+				rt_mutex_unlock(&q.pi_state->pi_mutex);
 			/*
 			 * Drop the reference to the pi state which
 			 * the requeue_pi() code acquired for us.

@@ -2905,6 +2906,8 @@ static int futex_wait_requeue_pi(u32 __u
 			spin_unlock(q.lock_ptr);
 		}
 	} else {
+		struct rt_mutex *pi_mutex;
+
 		/*
 		 * We have been woken up by futex_unlock_pi(), a timeout, or a
 		 * signal.  futex_unlock_pi() will not destroy the lock_ptr nor

kernel/futex.c +6 -8

+		 * userspace.
+		 */
+		if (ret && rt_mutex_owner(pi_mutex) == current)
+			rt_mutex_unlock(pi_mutex);
+
 		/* Unqueue and drop the lock. */
 		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