lkml.org 
[lkml]   [2009]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] ptrace: do not use task_lock() for attach
    On 05/06, Chris Wright wrote:
    >
    > * Oleg Nesterov (oleg@redhat.com) wrote:
    > > + write_lock_irq(&tasklist_lock);
    > > retval = -EPERM;
    > > if (unlikely(task->exit_state))
    > > - goto bad;
    > > + goto unlock_tasklist;
    > > if (task->ptrace)
    > > - goto bad;
    > > + goto unlock_tasklist;
    >
    > So, task->ptrace now protected by tasklist_lock to keep concurrent tracers
    > from both attaching to same task?

    Yes,

    > (re-oredered)
    >
    > What do you think?

    I think I need your help!

    > What does this do for setprocattr()?
    >
    > task_lock(p);
    > tracer = tracehook_tracer_task(p);
    > if (tracer)
    > ptsid = task_sid(tracer);
    > task_unlock(p);
    >
    > Looks like it is racy.

    Looks like you are right, but I don't understand selinux at all.

    > cpu1 (tracer) cpu2 (tracee, changing sid)
    > ------------- ---------------------------
    > task_lock(tracee);
    > __ptrace_may_access(tracee, ATTACH);
    > task_unlock(tracee);
    > task_lock(tracee)
    > <security check passes> tracer = tracehook_tracer_task(tracee);
    > if (tracer) <-- NULL, !tracee->ptrace
    > ...
    > update sid w/out checking against tracer
    > write_lock_irq(&tasklist_lock);
    > ...
    > tracee->ptrace = PT_PTRACED;
    > ...
    > now we are tracing task w/ a sid
    > that we didn't authorize to trace

    But this can happen without this change too?

    - cpu2 takes task_lock(), tracehook_tracer_task() returns NULL because
    we are not traced yet.

    - cpu1 does ptrace_attach() and succeds, because cpu2 didn't update sid
    yet

    - cpu2 continues, it doesn't check avc_has_perm() (tracer == 0) and
    updates sid.

    No?

    Shouldn't selinux_setprocattr() take ->cred_exec_mutex, like we do in
    selinux_bprm_set_creds() path?

    Oleg.



    \
     
     \ /
      Last update: 2009-05-07 01:23    [W:0.023 / U:0.508 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site