Messages in this thread | | | Date | Thu, 29 Sep 2011 21:05:18 -0400 | From | Stephen Wilson <> | Subject | Re: Q: proc: hold cred_guard_mutex in check_mem_permission() |
| |
On Thu, Sep 29, 2011 at 01:48:27PM +0200, Oleg Nesterov wrote: > > > 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? > > Yes, and that is why I sent 704b836c ;) > > But, check_mem_permission() can't race with exec, that was my point. > The task is stopped (it is TASK_TRACED), it can't run unless SIGKILL'ed. > It can not change its mm/creds.
Ah, I see. I misunderstood your question. So, yes, the race as it stands is fictional AFAIKT -- I did not recognize that the ptrace_may_access() was effectively redundant when I first authored the patch.
> > Checking ptrace_parent() against current was introduced in 0d094efeb, > > not actually, this is very old check, probably since 2.4.0 at least. > That patch only renames the helper we use. > > > but the > > commit message gives no clue as to why the check was added. > > Only debugger can read/write the task's memory. May be we can relax > this, perhaps we can only check ptrace_may_access(PTRACE_MODE_ATTACH). > > But currently we require the caller should trace the target.
OK.
> > If we are the tracer then that ptrace_may_access() check is redundant and the > > whole race thing is a non-issue, right? > > Yes. > > > But perhaps the correct move is to > > relax the restriction that current be the tracer. > > Yes, I thought about this too. And in this case we do need the mutex. > Although I don't think this really makes sense, I _think_ this all was > created for debuggers. And, without ptrace_parent(), why do we need > task_is_stopped_or_traced() check?
Thinking about this some more, I agree that this should remain only for debuggers -- /proc/pid/mem is just a nicer interface to ptrace peek and poke, nothing more.
> So I think we should simply remove ->cred_guard_mutex.
Yes, I think that is right, together with removing the ptrace_may_access() check and updating that comment in check_mem_permission(). I can put a patch together this weekend if needed.
Take care,
-- steve
| |