From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758181AbcLAExB (ORCPT ); Wed, 30 Nov 2016 23:53:01 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:42322 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754743AbcLAExA (ORCPT ); Wed, 30 Nov 2016 23:53:00 -0500 Date: Wed, 30 Nov 2016 20:55:30 -0800 From: Darren Hart To: Peter Zijlstra Cc: Thomas Gleixner , Ingo Molnar , linux-kernel@vger.kernel.org Subject: Re: [PATCH] futex: Fix potential use-after-free in FUTEX_REQUEUE_PI Message-ID: <20161201045530.GA124104@f23x64.localdomain> References: <20161124144235.GD3124@twins.programming.kicks-ass.net> <20161124153808.GH3092@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161124153808.GH3092@twins.programming.kicks-ass.net> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 24, 2016 at 04:38:08PM +0100, Peter Zijlstra wrote: > On Thu, Nov 24, 2016 at 04:19:41PM +0100, Thomas Gleixner wrote: > > On Thu, 24 Nov 2016, Peter Zijlstra wrote: > > > > > > > > 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) > > > --- > > > kernel/futex.c | 22 +++++++++++++--------- > > > 1 file changed, 13 insertions(+), 9 deletions(-) > > > > > > diff --git a/kernel/futex.c b/kernel/futex.c > > > index 2c4be467fecd..d5a81339209f 100644 > > > --- a/kernel/futex.c > > > +++ b/kernel/futex.c > > > @@ -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; > > > @@ -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 :/ Why would you have to drag it back out again? Something else you're working on? -- Darren Hart Intel Open Source Technology Center