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

Message-Id: <20100914100626.C981.A69D9226@jp.fujitsu.com>
Date: Thu, 16 Sep 2010 14:51:54 +0900 (JST)
From: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: kosaki.motohiro@...fujitsu.com, 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.

I see, thanks.

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

Good idea. will fix.

> 
> 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).

Thanks. will fix.


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

Will fix, of cource.

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.