linux-kernel - [PATCH] futex: Robustify wake

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: <1424148397.2046.101.camel@stgolabs.net>
Date:	Mon, 16 Feb 2015 20:46:37 -0800
From:	Davidlohr Bueso <dave@...olabs.net>
To:	tglx@...utronix.de, mingo@...nel.org
Cc:	peterz@...radead.org, dvhart@...ux.intel.com,
	linux-kernel@...r.kernel.org, dave@...olabs.net
Subject: [PATCH] futex: Robustify wake_futex()

Current code assumes that wake_futex() will never fail, thus
we are rather sloppy when incrementing the return value in wake
related calls, accounting for the newly woken task. Of course
this will never occur, thus not a problem. This bug is as real
as the need for the redundant pi checks in wake_futex().

These redundant checks are fine and past discussion indicates
that they will stay. However, it does introduce this mismatch,
thus it is better to robustify the function and avoid any
assumptions that could bite us in the arse the future.

Signed-off-by: Davidlohr Bueso <dbueso@...e.de>
---

This got my attention this while looking at Micheal's futex manpage
changes. The downside of this patch is that the return error from
wake_futex() is even more redundant now... however it still seems
like the right thing to do.

 kernel/futex.c | 45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 2a5e383..a01843a 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1091,13 +1091,16 @@ static void __unqueue_futex(struct futex_q *q)
 /*
  * The hash bucket lock must be held when this is called.
  * Afterwards, the futex_q must not be accessed.
+ *
+ * Returns true only when the task was woken, otherwise false.
  */
-static void wake_futex(struct futex_q *q)
+static bool wake_futex(struct futex_q *q)
 {
 	struct task_struct *p = q->task;
 
-	if (WARN(q->pi_state || q->rt_waiter, "refusing to wake PI futex\n"))
-		return;
+	if (unlikely(WARN(q->pi_state || q->rt_waiter,
+			  "refusing to wake PI futex\n")))
+		return false;
 
 	/*
 	 * We set q->lock_ptr = NULL _before_ we wake up the task. If
@@ -1120,6 +1123,7 @@ static void wake_futex(struct futex_q *q)
 
 	wake_up_state(p, TASK_NORMAL);
 	put_task_struct(p);
+	return true;
 }
 
 static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_q *this)
@@ -1244,7 +1248,10 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)
 			if (!(this->bitset & bitset))
 				continue;
 
-			wake_futex(this);
+			if (!wake_futex(this)) {
+				ret = -EINVAL;
+				break;
+			}
 			if (++ret >= nr_wake)
 				break;
 		}
@@ -1320,7 +1327,11 @@ retry_private:
 				ret = -EINVAL;
 				goto out_unlock;
 			}
-			wake_futex(this);
+
+			if (!wake_futex(this)) {
+				ret = -EINVAL;
+				goto out_unlock;
+			}
 			if (++ret >= nr_wake)
 				break;
 		}
@@ -1334,7 +1345,11 @@ retry_private:
 					ret = -EINVAL;
 					goto out_unlock;
 				}
-				wake_futex(this);
+
+				if (!wake_futex(this)) {
+					ret = -EINVAL;
+					goto out_unlock;
+				}
 				if (++op_ret >= nr_wake2)
 					break;
 			}
@@ -1674,13 +1689,19 @@ retry_private:
 		}
 
 		/*
-		 * Wake nr_wake waiters.  For requeue_pi, if we acquired the
-		 * lock, we already woke the top_waiter.  If not, it will be
-		 * woken by futex_unlock_pi().
+		 * For requeue_pi, if we acquired the lock, we already woke
+		 * the top_waiter. If not, it will be woken by futex_unlock_pi.
+		 *
+		 * The regular (non-pi) case is much simpler: Wake the top
+		 * waiter (next in line) and repeat.
 		 */
-		if (++task_count <= nr_wake && !requeue_pi) {
-			wake_futex(this);
-			continue;
+		if (!requeue_pi) {
+			if (!wake_futex(this)) {
+				ret = -EINVAL;
+				break;
+			}
+			if (++task_count <= nr_wake)
+				continue;
 		}
 
 		/* Ensure we requeue to the expected futex for requeue_pi. */
-- 
2.1.4



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/