lkml.org 
[lkml]   [2020]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] exec: Fix a deadlock in ptrace
    Date
    On 3/1/20 7:52 PM, Christian Brauner wrote:
    > On Sun, Mar 01, 2020 at 07:21:03PM +0100, Jann Horn wrote:
    >> On Sun, Mar 1, 2020 at 12:27 PM Bernd Edlinger
    >> <bernd.edlinger@hotmail.de> wrote:
    >>> The proposed solution is to have a second mutex that is
    >>> used in mm_access, so it is allowed to continue while the
    >>> dying threads are not yet terminated.
    >>
    >> Just for context: When I proposed something similar back in 2016,
    >> https://lore.kernel.org/linux-fsdevel/20161102181806.GB1112@redhat.com/
    >> was the resulting discussion thread. At least back then, I looked
    >> through the various existing users of cred_guard_mutex, and the only
    >> places that couldn't be converted to the new second mutex were
    >> PTRACE_ATTACH and SECCOMP_FILTER_FLAG_TSYNC.
    >>
    >>
    >> The ideal solution would IMO be something like this: Decide what the
    >> new task's credentials should be *before* reaching de_thread(),
    >> install them into a second cred* on the task (together with the new
    >> dumpability), drop the cred_guard_mutex, and let ptrace_may_access()
    >> check against both. After that, some further restructuring might even
    >
    > Hm, so essentially a private ptrace_access_cred member in task_struct?
    > That would presumably also involve altering various LSM hooks to look at
    > ptrace_access_cred.
    >
    > (Minor side-note, de_thread() takes a struct task_struct argument but
    > only ever is passed current.)
    >
    >> allow the cred_guard_mutex to not be held across all of the VFS
    >> operations that happen early on in execve, which may block
    >> indefinitely. But that would be pretty complicated, so I think your
    >> proposed solution makes sense for now, given that nobody has managed
    >> to implement anything better in the last few years.
    >
    > Reading through the old threads and how often this issue came up, I tend
    > to agree.
    >

    Okay, fine.

    I managed to change Oleg's test case, into one that shows what exactly
    is changed with this patch:


    $ cat t.c
    #include <stdio.h>
    #include <fcntl.h>
    #include <unistd.h>
    #include <pthread.h>
    #include <sys/signal.h>
    #include <sys/ptrace.h>

    void *thread(void *arg)
    {
    ptrace(PTRACE_TRACEME, 0,0,0);
    return NULL;
    }

    int main(void)
    {
    int f, pid = fork();
    char mm[64];

    if (!pid) {
    pthread_t pt;
    pthread_create(&pt, NULL, thread, NULL);
    pthread_join(pt, NULL);
    execlp("echo", "echo", "passed", NULL);
    }

    sleep(1);
    sprintf(mm, "/proc/%d/mem", pid);
    printf("open(%s)\n", mm);
    f = open(mm, O_RDONLY);
    printf("f = %d\n", f);
    // this is not fixed! ptrace(PTRACE_ATTACH, pid, 0,0);
    kill(pid, SIGCONT);
    if (f >= 0)
    close(f);
    return 0;
    }
    $ gcc -pthread -Wall t.c
    $ ./a.out
    open(/proc/2802/mem)
    f = 3
    $ passed

    previously this did block, how can I make a test case for this?
    I am not so experienced in this matter.


    Thanks
    Bernd.
    \
     
     \ /
      Last update: 2020-03-01 20:01    [W:4.636 / U:1.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site