linux-kernel - Re: futex: race in lock and unlock&exit for robust futex with PI?

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: <1277743748.3561.139.camel@laptop>
Date:	Mon, 28 Jun 2010 18:49:08 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Michal Hocko <mhocko@...e.cz>
Cc:	Darren Hart <dvhltc@...ibm.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Nick Piggin <npiggin@...e.de>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: futex: race in lock and unlock&exit for robust futex with PI?

On Mon, 2010-06-28 at 18:39 +0200, Michal Hocko wrote:
> Would something like the following be acceptable (just a compile
> tested without comments). It simply makes caller of lookup_pi_state to
> decide whether credentials should be checked.

So it was Ingo, who in c87e2837be8 (pi-futex:
futex_lock_pi/futex_unlock_pi support) introduced the euid checks:

+futex_find_get_task():
+       if ((current->euid != p->euid) && (current->euid != p->uid)) {
+               p = NULL;
+               goto out_unlock;
+       }

Ingo, do you remember the rationale behind that? It seems to be causing
grief when two different users contend on the same (shared) futex.

See the below proposed solution.

> diff --git a/kernel/futex.c b/kernel/futex.c
> index e7a35f1..dfe4b11 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -426,7 +426,7 @@ static void free_pi_state(struct futex_pi_state *pi_state)
>   * Look up the task based on what TID userspace gave us.
>   * We dont trust it.
>   */
> -static struct task_struct * futex_find_get_task(pid_t pid)
> +static struct task_struct * futex_find_get_task(pid_t pid, bool check_cred)
>  {
>         struct task_struct *p;
>         const struct cred *cred = current_cred(), *pcred;
> @@ -436,10 +436,12 @@ static struct task_struct * futex_find_get_task(pid_t pid)
>         if (!p) {
>                 p = ERR_PTR(-ESRCH);
>         } else {
> -               pcred = __task_cred(p);
> -               if (cred->euid != pcred->euid &&
> -                   cred->euid != pcred->uid)
> -                       p = ERR_PTR(-ESRCH);
> +               if (check_cred) {
> +                       pcred = __task_cred(p);
> +                       if (cred->euid != pcred->euid &&
> +                           cred->euid != pcred->uid)
> +                               p = ERR_PTR(-ESRCH);
> +               }
>                 else
>                         get_task_struct(p);
>         }
> @@ -506,7 +508,7 @@ void exit_pi_state_list(struct task_struct *curr)
>  
>  static int
>  lookup_pi_state(u32 uval, struct futex_hash_bucket *hb,
> -               union futex_key *key, struct futex_pi_state **ps)
> +               union futex_key *key, struct futex_pi_state **ps, bool check_cred)
>  {
>         struct futex_pi_state *pi_state = NULL;
>         struct futex_q *this, *next;
> @@ -563,7 +565,7 @@ lookup_pi_state(u32 uval, struct futex_hash_bucket *hb,
>          */
>         if (!pid)
>                 return -ESRCH;
> -       p = futex_find_get_task(pid);
> +       p = futex_find_get_task(pid, check_cred);
>         if (IS_ERR(p))
>                 return PTR_ERR(p);
>  
> @@ -705,7 +707,7 @@ retry:
>          * We dont have the lock. Look up the PI state (or create it if
>          * we are the first waiter):
>          */
> -       ret = lookup_pi_state(uval, hb, key, ps);
> +       ret = lookup_pi_state(uval, hb, key, ps, false);
>  
>         if (unlikely(ret)) {
>                 switch (ret) {
> @@ -1258,7 +1260,7 @@ retry_private:
>                         ret = get_futex_value_locked(&curval2, uaddr2);
>                         if (!ret)
>                                 ret = lookup_pi_state(curval2, hb2, &key2,
> -                                                     &pi_state);
> +                                                     &pi_state, true);
>                 }
>  
>                 switch (ret) { 
--
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/