[lkml]   [2011]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: Q: proc: hold cred_guard_mutex in check_mem_permission()
    On 09/29, Stephen Wilson wrote:
    > 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?

    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.

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

    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.

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

    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?

    So I think we should simply remove ->cred_guard_mutex.


     \ /
      Last update: 2011-09-29 13:55    [W:0.025 / U:4.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site