linux-kernel - Re: [PATCH] futex: futex_find_get_task make credentials check conditional

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: <AANLkTinyoyZVg8iYOuUx6aA4XlUlLywl4q6GLXmeYP-x@mail.gmail.com>
Date:	Tue, 29 Jun 2010 09:41:02 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Michal Hocko <mhocko@...e.cz>
Cc:	Ingo Molnar <mingo@...e.hu>, 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>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] futex: futex_find_get_task make credentials check 
	conditional

On Tue, Jun 29, 2010 at 1:42 AM, Michal Hocko <mhocko@...e.cz> wrote:
>
> futex_find_get_task is currently used (through lookup_pi_state) from two
> contexts, futex_requeue and futex_lock_pi_atomic. While credentials check
> makes sense in the first code path, the second one is more problematic
> because this check requires that the PI lock holder (pid parameter) has
> the same uid and euid as the process's euid which is trying to lock the
> same futex (current).

So exactly why does it make sense to check the credentials in the
first code path then? Shouldn't the futex issue in the end depend on
whether you have a shared page or not - and not on credentials at all?
Any two processes that share a futex in the same shared page should be
able to use that without any regard for whether they are the same
user. That's kind of the point, no?

IOW, I personally dislike these kinds of conditional checks,
especially since the discussion (at least the part I've seen) hasn't
made it clear why it should be conditional - or exist - in the first
place.

So I'd like the patch to include an explanation of exactly why the two
cases are different.

The other thing I'd like to see is to move the whole cred checking up
a level. There's no reason to check the credentials in
futex_find_get_task() that I can see - why not do it in the caller
instead? IOW, I think futex_find_get_task() should just look something
like this instead:

  static struct task_struct * futex_find_get_task(pid_t pid)
  {
        struct task_struct *p;

        rcu_read_lock();
        p = find_task_by_vpid(pid);
        if (p)
                get_task_struct(p);
        rcu_read_unlock();

        return p;
  }

and then in the caller we'd do something like

      p = futex_find_get_task(pid);
      if (!p)
         return -ESEARCH;

      if ( .. check p credentials is necessary and fails..)
         goto put_task_and_exit;

because especially with not everybody needing the credentials check, I
do not think it should be done at the lowest level (it's clearly not
fundamental to the operation, so it shouldn't be part of the core
lookup).

With some re-factoring, it might even be possible to avoid a dynamic
check at all, and just have two different static paths for the two
cases. That's a separate issue, though.

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