linux-kernel - Re: [PATCH] cred - synchronize rcu before releasing cred

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: <AANLkTikB9Msot4m_oaw+V+dvEg+SzyXHb0cqhKQqUtC2@mail.gmail.com>
Date:	Tue, 27 Jul 2010 10:56:20 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	David Howells <dhowells@...hat.com>
Cc:	Jiri Olsa <jolsa@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Eric Dumazet <eric.dumazet@...il.com>,
	linux-kernel@...r.kernel.org,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH] cred - synchronize rcu before releasing cred

On Tue, Jul 27, 2010 at 9:46 AM, David Howells <dhowells@...hat.com> wrote:
>
> That's not the problem.
>
> The problem is that task_state() accesses the target task's credentials whilst
> only holding the RCU read lock.  That means that the existence of the cred
> struct so accessed can only be guaranteed up to the point that the RCU read
> lock is released.

Umm. In that case, get_task_cred() is actively misleading.

What you are saying is that you cannot do

   rcu_read_lock()
   __cred = (struct cred *) __task_cred((task));
   get_cred(__cred);
   rcu_read_unlock();

but that is _exactly_ what get_task_cred() does. And that
__task_cred() check checks that we have

   rcu_read_lock_held() || lockdep_tasklist_lock_is_held()

and what you are describing would require us to have a '&&' rather
than a '||' in that test. Because it is _not_ sufficient to have just
the rcu_read_lock held.

So it looks like the validation is simply wrong. The __task_cred()
helper is buggy. It's used for two different cases, and they have
totally different locking requirements.

Case #1:
 - you can do __task_cred() with just read-lock held, but then you
cannot add refs to it

Case #2:
 - you can do __task_cred() with read-lock held _and_ guaranteeing
that the task doesn't go away, and then you can hold a ref to it as
long as you still guarantee the task is around.

And the comments are actively wrong. The comments talk about the "case
#2" thing only. Ignoring case #1, except for the fact that the _check_
allows case #1, so you never get a warning from the RCU "proving" code
even for incorrect code.

So presumably Jiri's patch is correct, but the reason the bug happened
in the first place is that all those accessor functions are totally
confused about how they supposed to be used, with incorrect comments
and incorrect access checks.

That should get fixed. Who knows how many other buggy users there are
due to the confusion?

                   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/