Messages in this thread |  | | | Date | Sun, 29 Mar 2009 23:36:35 +0200 | | From | Oleg Nesterov <> | | Subject | Re: 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 check_unsafe_exec().
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,
selinux_bprm_set_creds:
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; } ... zap_other_threads();
Oh, ugly.
I'd better hope I missed something ;)
Oleg.
|  |