linux-kernel - Re: Q: proc: hold cred_guard_mutex in check_mem

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: <20110929071314.GA7734@wicker.gateway.2wire.net>
Date:	Thu, 29 Sep 2011 03:13:14 -0400
From:	Stephen Wilson <wilsons@...rt.ca>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Stephen Wilson <wilsons@...rt.ca>,
	Al Viro <viro@...iv.linux.org.uk>,
	Johannes Weiner <jweiner@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Q: proc: hold cred_guard_mutex in check_mem_permission()

On Wed, Sep 28, 2011 at 10:20:20PM +0200, Oleg Nesterov wrote:
> Another change we probably need to backport, 18f661bc
> "proc: hold cred_guard_mutex in check_mem_permission()".
> 
> From the changelog:
> 
>     Avoid a potential race when task exec's and we get a new ->mm but
>     check against the old credentials in ptrace_may_access().
> 
> Could you please explain? How can we race with exec?

My understanding of the race is this:

sequence during execve():

	1) cred_guard_mutex is taken in prepare_bprm_creds()
	2) new mm is installed via exec_mmap()
	3) new creds are pushed via install_exec_creds() which releases
	   cred_guard_mutex

so if we get_task_mm() and ptrace_may_access() between 2 and 3 we
obtain a reference to the new mm validated against old creds.

Perhaps (the fairly old) commit 704b836c helps? 


> This task is either current, or it is TASK_TRACED and we are the
> tracer. In the latter case nobody can resume it except SIGKILL.
> And the killed task obviously can't exec.
> 
> Afaics, all we need is: we should read task->mm after the
> task_is_traced() check, we do not need the mutex.

Checking ptrace_parent() against current was introduced in 0d094efeb,  but the
commit message gives no clue as to why the check was added.  It does seem to
go against the comment "would also be permitted to freshly attach with
ptrace".  Not sure what to think ATM..

> IOW, what do you think about the patch below? I have no idea how
> can I test it (and it wasn't even applied/compiled).
>
> Also, I'd appreciate if you can explain the PTRACE_MODE_ATTACH
> check. Again, we are already the tracer.

If we are the tracer then that ptrace_may_access() check is redundant and the
whole race thing is a non-issue, right?  But perhaps the correct move is to
relax the restriction that current be the tracer.  I might be overlooking
something though.


Thanks,


-- 
steve

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