[lkml]   [2009]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't)
    On 03/29, Al Viro wrote:
    > > In principle, we can mark these threads as "-EAGAIN on such clone()" and
    > > clean that on exec failure.

    We can't. We can miss the new subthread if we race with clone(CLONE_THREAD).
    Unless we add the additional locking, of course.

    We can set current->signal->flags |= SIGNAL_DO_NOT_CLONE_FS. But this is
    really nasty. For examlpe, what if this flag is already set when
    check_unsafe_exec() takes ->siglock ? We should return -ESOMETHING, not
    good. Or schedule_timeout_uninterruptible(1) until it is cleared?

    This also means copy_process()->copy_fs() path should take ->siglock too,
    otherwise we we don't have a barrier.

    > ... or just do that to fs_struct. After finding that there's no outside
    > users. Commenst?

    This is even worse. Not only we race with our sub-threads, we race
    with CLONE_FS processes.

    We can't mark fs_struct after finding that there's no outside users
    lockless. Because we can't know whether this is "after" or not, we
    can't trust "atomic_read(fs->count) <= n_fs".

    Unless we re-use fs_struct->lock. In this case copy_fs() should take
    it too. But again, ->fs can be already marked when we enter

    And btw check_unsafe_exec() seem to have another hole. Another thread
    (which shares ->fs with us) can do exit_fs() right before we read
    fs->count. Since this thread was already accounted in n_fs, we can
    miss the fact we share ->fs with another process.

    Perhaps I missed something...

    Not that I like this idea (actually I hate), but perhaps we can change
    the meaning of LSM_UNSAFE_SHARE,


    if (new_tsec->sid != old_tsec->sid) {

    if (avc_has_perm(...))
    bprm->unsafe |= LSM_UNSAFE_SHARE;

    Then we modify de_thread(). It sends SIGKILL to all subthreads, this
    means that another thread can't clone() after we drop ->siglock. So we
    can add this code to the ->siglock protected section

    if (unlikely(bprm->unsafe & LSM_UNSAFE_SHARE)) {
    if (fs_struct_is_shared())
    return -EPERM;


    Oh, ugly.

    I'd better hope I missed something ;)


     \ /
      Last update: 2009-03-29 23:45    [W:0.022 / U:15.764 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site