linux-kernel - Re: [PATCH v3] fix race copy_process() vs de

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] [day] [month] [year] [list]

Message-ID: <4A932BF6.1090405@ct.jp.nec.com>
Date:	Tue, 25 Aug 2009 09:10:30 +0900
From:	Hiroshi Shimamoto <h-shimamoto@...jp.nec.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Roland McGrath <roland@...hat.com>,
	linux-kernel@...r.kernel.org,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	stable@...nel.org
Subject: Re: [PATCH v3] fix race copy_process() vs de_thread()

Oleg Nesterov wrote:
> Spotted by Hiroshi Shimamoto who also provided the test-case below.
> 
> copy_process() pathes use signal->count as a reference counter, but
> it is not. This test case
> 
> 	#include <sys/types.h>
> 	#include <sys/wait.h>
> 	#include <unistd.h>
> 	#include <stdio.h>
> 	#include <errno.h>
> 	#include <pthread.h>
> 
> 	void *null_thread(void *p)
> 	{
> 		for (;;)
> 			sleep(1);
> 
> 		return NULL;
> 	}
> 
> 	void *exec_thread(void *p)
> 	{
> 		execl("/bin/true", "/bin/true", NULL);
> 
> 		return null_thread(p);
> 	}
> 
> 	int main(int argc, char **argv)
> 	{
> 		for (;;) {
> 			pid_t pid;
> 			int ret, status;
> 
> 			pid = fork();
> 			if (pid < 0)
> 				break;
> 
> 			if (!pid) {
> 				pthread_t tid;
> 
> 				pthread_create(&tid, NULL, exec_thread, NULL);
> 				for (;;)
> 					pthread_create(&tid, NULL, null_thread, NULL);
> 			}
> 
> 			do {
> 				ret = waitpid(pid, &status, 0);
> 			} while (ret == -1 && errno == EINTR);
> 		}
> 
> 		return 0;
> 	}
> 
> quickly creates the unkillable task.
> 
> If copy_process(CLONE_THREAD) races with de_thread()
> copy_signal()->atomic(signal->count) breaks the signal->notify_count
> logic, and the execing thread can hang forever in kernel space.
> 
> Change copy_process() to increment count/live only when we know for
> sure we can't fail. In this case the forked thread will take care
> of its reference to signal correctly.
> 
> If copy_process() fails, check CLONE_THREAD flag. If it it set - do
> nothing, the counters were not changed and current belongs to the same
> thread group. If it is not set, ->signal must be released in any case
> (and ->count must be == 1), the forked child is the only thread in the
> thread group.
> 
> We need more cleanups here, in particular signal->count should not be
> used by de_thread/__exit_signal at all. This patch only fixes the bug.
> 
> Reported-by: Hiroshi Shimamoto <h-shimamoto@...jp.nec.com>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>

Nice fix!
Tested-by: Hiroshi Shimamoto <h-shimamoto@...jp.nec.com>

Thanks,
Hiroshi
--
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/