oss-security - Re: [PATCH] move cred_guard_mutex from task_struct to signal

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100910172457.GA23393@redhat.com>
Date: Fri, 10 Sep 2010 19:24:57 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc: Roland McGrath <roland@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, oss-security@...ts.openwall.com,
        Solar Designer <solar@...nwall.com>,
        Kees Cook <kees.cook@...onical.com>, Al Viro <viro@...iv.linux.org.uk>,
        Neil Horman <nhorman@...driver.com>, linux-fsdevel@...r.kernel.org,
        pageexec@...email.hu,
        "Brad Spengler <spender@...ecurity.net>, Eugene Teo" <eugene@...hat.com>,
        KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [PATCH] move cred_guard_mutex from task_struct to signal_struct

On 09/10, KOSAKI Motohiro wrote:
>
> 1) moving cread_guard_mutex itself
>    - no increase execve overhead
> 	-> very good
>    - it also prevent parallel ptrace

No, it doesn't. Only PTRACE_ATTACH needs this mutex, and as Roland
pointed out it also needs write_lock(tasklist) which is worse. So
this change doesn't make any practical harm for ptrace.

> 2) move in_exec_mm to signal_struct too
>    -> very hard. oom-killer can use very few lock because it's called
>       from various place. now both ->mm and ->in_exec_mm are protected
>       task_lock() and it help to avoid messy.

Yes. But, if ->in_exec_mm is only used by oom_badness(), then I think
you can use task_lock(tsk->group_leader). oom_badness() needs tasklist
anyway, this means it can't race with de_thread() changing the leader.
But up to you.

Another very minor nit (but again, up to you). Perhaps exec_mmap()
could clear ->in_exec_mm (in task_struct or signal_struct, this doesnt
matter), it takes task_lock(current) anyway (and at this point current
is always the group leader).

> Let's move ->cred_guard_mutex from task_struct to signal_struct. It
> naturally prevent multiple-threads-inside-exec.

Reviewed-by: Oleg Nesterov <oleg@...hat.com>

This is very minor, but perhaps you can also fix a couple of comments
which mention task->cred_guard_mutex,

	fs/exec.c:1109		the caller must hold current->cred_guard_mutex
	kernel/cred.c:328	The caller must hold current->cred_guard_mutex
	include/linux/tracehook.h:153	@task->cred_guard_mutex

Oleg.

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.