lkml.org 
[lkml]   [2020]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCHv2] exec: Fix a deadlock in ptrace
    Date
    On 3/2/20 5:43 PM, Jann Horn wrote:
    > On Mon, Mar 2, 2020 at 5:19 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
    >>
    >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
    >>
    >>> On 3/2/20 4:57 PM, Eric W. Biederman wrote:
    >>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
    >>>>
    >>>>>
    >>>>> I tried this with s/EACCESS/EACCES/.
    >>>>>
    >>>>> The test case in this patch is not fixed, but strace does not freeze,
    >>>>> at least with my setup where it did freeze repeatable.
    >>>>
    >>>> Thanks, That is what I was aiming at.
    >>>>
    >>>> So we have one method we can pursue to fix this in practice.
    >>>>
    >>>>> That is
    >>>>> obviously because it bypasses the cred_guard_mutex. But all other
    >>>>> process that access this file still freeze, and cannot be
    >>>>> interrupted except with kill -9.
    >>>>>
    >>>>> However that smells like a denial of service, that this
    >>>>> simple test case which can be executed by guest, creates a /proc/$pid/mem
    >>>>> that freezes any process, even root, when it looks at it.
    >>>>> I mean: "ln -s README /proc/$pid/mem" would be a nice bomb.
    >>>>
    >>>> Yes. Your the test case in your patch a variant of the original
    >>>> problem.
    >>>>
    >>>>
    >>>> I have been staring at this trying to understand the fundamentals of the
    >>>> original deeper problem.
    >>>>
    >>>> The current scope of cred_guard_mutex in exec is because being ptraced
    >>>> causes suid exec to act differently. So we need to know early if we are
    >>>> ptraced.
    >>>>
    >>>
    >>> It has a second use, that it prevents two threads entering execve,
    >>> which would probably result in disaster.
    >>
    >> Exec can fail with an error code up until de_thread. de_thread causes
    >> exec to fail with the error code -EAGAIN for the second thread to get
    >> into de_thread.
    >>
    >> So no. The cred_guard_mutex is not needed for that case at all.
    >>
    >>>> If that case did not exist we could reduce the scope of the
    >>>> cred_guard_mutex in exec to where your patch puts the cred_change_mutex.
    >>>>
    >>>> I am starting to think reworking how we deal with ptrace and exec is the
    >>>> way to solve this problem.
    >>
    >>
    >> I am 99% convinced that the fix is to move cred_guard_mutex down.
    >
    > "move cred_guard_mutex down" as in "take it once we've already set up
    > the new process, past the point of no return"?
    >
    >> Then right after we take cred_guard_mutex do:
    >> if (ptraced) {
    >> use_original_creds();
    >> }
    >>
    >> And call it a day.
    >>
    >> The details suck but I am 99% certain that would solve everyones
    >> problems, and not be too bad to audit either.
    >
    > Ah, hmm, that sounds like it'll work fine at least when no LSMs are involved.
    >
    > SELinux normally doesn't do the execution-degrading thing, it just
    > blocks the execution completely - see their selinux_bprm_set_creds()
    > hook. So I think they'd still need to set some state on the task that
    > says "we're currently in the middle of an execution where the target
    > task will run in context X", and then check against that in the
    > ptrace_may_access hook. Or I suppose they could just kill the task
    > near the end of execve, although that'd be kinda ugly.
    >

    We have current->in_execve for that, right?
    I think when the cred_guard_mutex is taken only in the critical section,
    then PTRACE_ATTACH could take the guard_mutex, and look at current->in_execve,
    and just return -EAGAIN in that case, right, everybody happy :)


    Bernd.
    \
     
     \ /
      Last update: 2020-03-02 18:02    [W:4.643 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site