linux-kernel - Re: [RESEND PATCH] futex: fix key reference counter in case of 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: <4CB8A7EB.6050303@linux.intel.com>
Date:	Fri, 15 Oct 2010 12:13:47 -0700
From:	Darren Hart <dvhart@...ux.intel.com>
To:	Louis Rilling <louis.rilling@...labs.com>
CC:	linux-kernel@...r.kernel.org,
	Rusty Russell <rusty@...tcorp.com.au>,
	Ingo Molnar <mingo@...e.hu>,
	Thomas Gleixner <tglx@...utronix.de>,
	Matthieu Fertré <matthieu.fertre@...labs.com>
Subject: Re: [RESEND PATCH] futex: fix key reference counter in case of requeue.

On 10/14/2010 04:30 AM, Louis Rilling wrote:
> From: Matthieu Fertré<matthieu.fertre@...labs.com>

Hi Matthew,

>
> This patch ensures that we are referring to the right key when dropping
> reference for the futex_wait operation.
>
> The following scenario explains a typical case where the bug was
> happening:
>
> Process P calls futex_wait() on futex identified by 'key1'. 2 references
> are taken on this key: one for the struct futex_q itself, and one for the
> futex_wait operation.
> If now, process P is requeued on a futex identified by 'key2', its
> futex_q->key is updated from 'key1' to 'key2' and a reference is got
> to 'key2' and one is dropped to 'key1'.
> Later, another process calls futex_wake(): it gets a reference to
> 'key2', wakes process P, and drops reference to 'key2'.
> Once process P is woken up, it should unqueue, drop reference to 'key2'
> (the one referring to the futex_q, this is done in unqueue_me())
> and to 'key1' (the one referring to futex_wait operation). Without this
> patch it drops reference to 'key2' instead of 'key1'.

Nice catch. How did this manifest itself? Did you catch it just by code 
inspection?

I've been trying to develop a futex test suite to catch issues with the 
futex implementation, as well as to test any changes made to avoid 
regressions. Mind having a look?

http://git.kernel.org/?p=linux/kernel/git/dvhart/futextest.git;a=summary

> Signed-off-by: Matthieu Fertré<matthieu.fertre@...labs.com>
> Signed-off-by: Louis Rilling<louis.rilling@...labs.com>
> ---
>   kernel/futex.c |    8 ++++++--
>   1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 6a3a5fa..bed6717 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1791,6 +1791,7 @@ static int futex_wait(u32 __user *uaddr, int fshared,
>   	struct restart_block *restart;
>   	struct futex_hash_bucket *hb;
>   	struct futex_q q;
> +	union futex_key key;

We should be able to do this properly without requiring an additional 
key variable. I think tglx has proposed a suitable fix - but it needs 
testing to avoid any subtle regressions.

-- 
Darren Hart
Embedded 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/