lkml.org 
[lkml]   [2011]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: 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



    \
     
     \ /
      Last update: 2011-09-30 03:07    [W:0.023 / U:1.700 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site