linux-kernel - Re: [PATCH 2/2] mm/linux-next: Fix rcu locking in vm_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: <20120307153849.GA18879@redhat.com>
Date:	Wed, 7 Mar 2012 16:38:49 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Siddhesh Poyarekar <siddhesh.poyarekar@...il.com>
Cc:	Mark Salter <msalter@...hat.com>,
	linux-next <linux-next@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Paul Gortmaker <paul.gortmaker@...driver.com>
Subject: Re: [PATCH 2/2] mm/linux-next: Fix rcu locking in vm_is_stack

On 03/07, Siddhesh Poyarekar wrote:
>
> Take rcu read lock before we do anything at all with the threadgroup
> list. Also use list_first_entry_rcu to safely get the reference to the
> first task in the list.

Sorry, but both patches are absolutely wrong, afaics. This change fixes
nothing, just obfuscates the code.

Probably my explanation was not clear, let me try again.

rcu_read_lock() can not help without the additional checks. By the
time you take it, task->thread_group->next can point to nowhere.

So,

> @@ -3932,18 +3932,20 @@ pid_t vm_is_stack(struct task_struct *task,
>  		return task->pid;
>
>  	if (in_group) {
> -		struct task_struct *t = task;
> +		struct task_struct *t;
>  		rcu_read_lock();
> -		while_each_thread(task, t) {
> +		t = list_first_entry_rcu(&task->thread_group,
> +					 struct task_struct, thread_group);

It is not safe to dereference this pointer.

Once again. You have the task_struct *task. It exits,
but task->thread_group->next still points to another thread T. Now suppose
that T exits too. But task->thread_group->next was not changed, it still
points to T. RCU grace period passes, T is freed.

After that you take rcu_read_lock(), but it is too late! >next points to
the already freed/reused memory. How can list_first_entry_rcu() help?

What you need is something like

		rcu_read_lock();
		if (!pid_alive(task))
			goto done;

		t = task;
		do {
			...
		} while_each(task, t);

	done:
		rcu_read_unlock();

And. Imho it is not good to have the (afaics exactly?) same code in
mm/nommu.c, even with the same names. Why it is not possible to make
a single definition?

Oleg.

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