linux-kernel - Re: KASAN: use-after-free Read in task_is

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: <20181025115245.GB3725@redhat.com>
Date:   Thu, 25 Oct 2018 13:52:46 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        syzbot <syzbot+a9ac39bf55329e206219@...kaller.appspotmail.com>,
        James Morris <jmorris@...ei.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        syzkaller-bugs@...glegroups.com
Subject: Re: KASAN: use-after-free Read in task_is_descendant

On 10/25, Kees Cook wrote:
>
> task_is_descendant() is called under rcu_read_lock() in both
> ptracer_exception_found() and yama_ptrace_access_check() so I don't
> understand how any of the tasks could get freed? This is walking
> group_leader and real_parent -- are these not stable under rcu_lock()?

group_leader/real_parent/etc are no longer rcu-protected after the exiting
child calls release_task() which in particular removes the child from
children/thread_group lists.

OK. Suppose you have an rcu-protected list, and each element also has a
reference counter so you can do something

	struct elt {
		atomic_t ctr;
		struct list_head list;
		int pid;
	};

	rcu_read_lock();
	list_for_each_entry(elt, &LIST, list) {
		if (elt->pid == 100) {
			atomic_inc(&elt->ctr);	// get_task_struct()
			break;
		}
	}
	rcu_read_unlock();

	do_something(elt);

This code is fine. This elt can't be freed, you have a reference. But once
you drop rcu lock you can't trust elt->list.next! So, for example, you can
not do

	rcu_read_lock();
	list_for_each_entry_continue_rcu(elt, &LIST, list) {
		...
	}
	rcu_read_unlock();

too late, elt.list.next can be already freed, or it can be freed while you
iterate the list.

Another simple example. Suppose you have a global PTR protected by rcu. So
ignoring the necessary rcu_dereference this code is fine:

	rcu_read_lock();
	if (ptr = PTR)
		do_something(ptr);
	rcu_read_unlcok();

But this is not:

	ptr = PTR;
	rcu_read_lock();
	if (ptr)
		do_something(ptr);
	rcu_read_unlock();

basically the same thing...

Oleg.