linux-kernel - Re: [PATCH] fork: fix error handling in dup

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]

Date:	Thu, 12 Jul 2012 15:06:23 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Akinobu Mita <akinobu.mita@...il.com>
Cc:	linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	David Howells <dhowells@...hat.com>,
	Koichi Yasutake <yasutake.koichi@...panasonic.com>,
	linux-am33-list@...hat.com, Paul Mundt <lethal@...ux-sh.org>,
	linux-sh@...r.kernel.org, Chris Metcalf <cmetcalf@...era.com>,
	Salman Qazi <sqazi@...gle.com>
Subject: Re: [PATCH] fork: fix error handling in dup_task()

On Thu, 12 Jul 2012 20:04:53 +0900
Akinobu Mita <akinobu.mita@...il.com> wrote:

> The function dup_task() may fail at the following function calls in
> the following order.
> 
> 0) alloc_task_struct_node()
> 1) alloc_thread_info_node()
> 2) arch_dup_task_struct()
> 
> Error by 0) is not a matter, it can just return.  But error by 1)
> requires releasing task_struct allocated by 0) before it returns.
> Likewise, error by 2) requires releasing task_struct and thread_info
> allocated by 0) and 1).
> 
> The existing error handling calls free_task_struct() and
> free_thread_info() which do not only release task_struct and
> thread_info, but also call architecture specific
> arch_release_task_struct() and arch_release_thread_info().
> 
> The problem is that task_struct and thread_info are not fully
> initialized yet at this point, but arch_release_task_struct() and
> arch_release_thread_info() are called with them.
> 
> For example, x86 defines its own arch_release_task_struct() that
> releases a task_xstate.  If alloc_thread_info_node() fails in
> dup_task(), arch_release_task_struct() is called with task_struct
> which is just allocated and filled with garbage in this error handling.
> 
> This actually happened with tools/testing/fault-injection/failcmd.sh
> 
> 	# env FAILCMD_TYPE=fail_page_alloc \
> 		./tools/testing/fault-injection/failcmd.sh --times=100 \
> 		--min-order=0 --ignore-gfp-wait=0 \
> 		-- make -C tools/testing/selftests/ run_tests
> 
> In order to fix this issue, make free_{task_struct,thread_info}() not
> to call arch_release_{task_struct,thread_info}() and call
> arch_release_{task_struct,thread_info}() implicitly where needed.
> 
> Default arch_release_task_struct() and arch_release_thread_info() are
> defined as empty by default.  So this change only affects the
> architectures which implement their own arch_release_task_struct() or
> arch_release_thread_info() as listed below.

This conflicts with Salman's fix (below) which is in linux-next via
Ingo's tree.

It appears that we should drop Salman's patch altogether and use yours?


commit 164c33c6adee609b8b9062cce4c10f764d0dce13
Author:     Salman Qazi <sqazi@...gle.com>
AuthorDate: Mon Jun 25 18:18:15 2012 -0700
Commit:     Ingo Molnar <mingo@...nel.org>
CommitDate: Thu Jul 5 20:57:32 2012 +0200

    sched: Fix fork() error path to not crash
    
    In dup_task_struct(), if arch_dup_task_struct() fails, the clean up
    code fails to clean up correctly.  That's because the clean up
    code depends on unininitalized ti->task pointer.  We fix this
    by making sure that the task and thread_info know about each other
    before we attempt to take the error path.
    
    Signed-off-by: Salman Qazi <sqazi@...gle.com>
    Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
    Link: http://lkml.kernel.org/r/20120626011815.11323.5533.stgit@dungbeetle.mtv.corp.google.com
    Signed-off-by: Ingo Molnar <mingo@...nel.org>

diff --git a/kernel/fork.c b/kernel/fork.c
index ab5211b..f00e319 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -304,12 +304,17 @@ static struct task_struct *dup_task_struct(struct task_struct *orig)
 	}
 
 	err = arch_dup_task_struct(tsk, orig);
-	if (err)
-		goto out;
 
+	/*
+	 * We defer looking at err, because we will need this setup
+	 * for the clean up path to work correctly.
+	 */
 	tsk->stack = ti;
-
 	setup_thread_stack(tsk, orig);
+
+	if (err)
+		goto out;
+
 	clear_user_return_notifier(tsk);
 	clear_tsk_need_resched(tsk);
 	stackend = end_of_stack(tsk);

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