linux-kernel - Re: 3.5-rc6 futex_wait_requeue

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-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <50090079.1000703@linux.intel.com>
Date:	Thu, 19 Jul 2012 23:53:45 -0700
From:	Darren Hart <dvhart@...ux.intel.com>
To:	Dave Jones <davej@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Darren Hart <darren@...art.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: 3.5-rc6 futex_wait_requeue_pi oops.

On 07/19/2012 05:37 PM, Darren Hart wrote:
> 
> 
> On 07/19/2012 04:22 PM, Darren Hart wrote:
>>
>>
>> On 07/13/2012 11:54 AM, Dave Jones wrote:
>>> On Fri, Jul 13, 2012 at 08:47:38PM +0200, Thomas Gleixner wrote:
>>>  > On Fri, 13 Jul 2012, Dave Jones wrote:
>>>  > 
>>>  > > Looks like calling futex() with garbage makes things unhappy.
>>>  > 
>>>  >                 WARN_ON(!&q.pi_state);
>>>  >                 pi_mutex = &q.pi_state->pi_mutex;
>>>  >                 ret = rt_mutex_finish_proxy_lock(pi_mutex, to, &rt_waiter, 1);
>>>  >                 debug_rt_mutex_free_waiter(&rt_waiter);
>>>  > 
>>>  > So there is some weird way which causes q.pi_state = NULL. Dave, did
>>>  > you see the warning before the oops happened ?
>>>
>>> No, that didn't seem to trigger.
>>
>> Well I don't have a fix yet, but I can explain this not triggering.
>>
>> q is on the stack, so the ADDRESS for q.pi_state is never going to be
>> NULL. However, properly instrumented, we do see this:
>>
>> [   23.621501] ---[ end trace 20bdfb44db182a17 ]---
>> [   23.622425] q.pi_state @           (null)
>> [   23.623272] &q.pi_state @ ffff880185e2dca8
>> [   23.624119] ------------[ cut here ]------------
>>
>> Duh.
>>
>> I'll add a fix to that WARN_ON in my futex-fixes branch along with the
>> fix for the bug Dan found.
>>
> 
> I think I have root cause. futex_wait_requeue_pi() doesn't like having
> uaddr == uaddr2. The handle_early_wakeup() doesn't detect a problem
> because key2 IS the same as key1, I think. I've just discovered this and
> quickly hacked in a "if (uaddr==uaddr2) return -EINVAL" fix and the test
> continues to run (with just ops 0, 11, 12) for several minutes now
> (typically fails in a few seconds). I'll let it run for a few hours and
> contemplate the proper fix.

Dave, mind giving this a spin? It seems to be doing the trick here,
at least for the *REQUEUE_PI futex op codes in trinity.

 
>From d689b1598d67520dd87b30cc1ce7c6b76f566f43 Mon Sep 17 00:00:00 2001
Message-Id: <d689b1598d67520dd87b30cc1ce7c6b76f566f43.1342766842.git.dvhart@...ux.intel.com>
From: Darren Hart <dvhart@...ux.intel.com>
Date: Thu, 19 Jul 2012 23:40:15 -0700
Subject: [PATCH] futex: Forbid uaddr == uaddr2 in futex_wait_requeue_pi()

If uaddr == uaddr2, then we have broken the rule of only requeueing from
a non-pi futex to a pi futex with this call. If we attempt this, as the
trinity test suite manages to do, we miss early wakeups as q.key is
equal to key2 (because they are the same uaddr). We will then attempt to
dereference the pi_mutex (which would exist had the futex_q been
properly requeued to a pi futex) and trigger a NULL pointer dereference.

Signed-off-by: Darren Hart <dvhart@...ux.intel.com>
CC: Dave Jones <davej@...hat.com>
CC: Thomas Gleixner <tglx@...utronix.de>
---
 kernel/futex.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 5551ada..3717e7b 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -2231,11 +2231,11 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
  * @uaddr2:	the pi futex we will take prior to returning to user-space
  *
  * The caller will wait on uaddr and will be requeued by futex_requeue() to
- * uaddr2 which must be PI aware.  Normal wakeup will wake on uaddr2 and
- * complete the acquisition of the rt_mutex prior to returning to userspace.
- * This ensures the rt_mutex maintains an owner when it has waiters; without
- * one, the pi logic wouldn't know which task to boost/deboost, if there was a
- * need to.
+ * uaddr2 which must be PI aware and unique from uaddr.  Normal wakeup will wake
+ * on uaddr2 and complete the acquisition of the rt_mutex prior to returning to
+ * userspace.  This ensures the rt_mutex maintains an owner when it has waiters;
+ * without one, the pi logic would not know which task to boost/deboost, if
+ * there was a need to.
  *
  * We call schedule in futex_wait_queue_me() when we enqueue and return there
  * via the following:
@@ -2272,6 +2272,9 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 	struct futex_q q = futex_q_init;
 	int res, ret;
 
+	if (uaddr == uaddr2)
+		return -EINVAL;
+
 	if (!bitset)
 		return -EINVAL;
 
-- 
1.7.5.4


-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel


--
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/