Re: Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't)

[Al Viro <viro@ZenIV.linux.org.uk>]

On Mon, Mar 30, 2009 at 02:03:38AM +0200, Oleg Nesterov wrote:
> On 03/30, Oleg Nesterov wrote:
> >
> > On 03/29, Al Viro wrote:
> > >
> > > On Sun, Mar 29, 2009 at 11:36:35PM +0200, Oleg Nesterov wrote:
> > > > > ... 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".
> > >
> > > We can lock fs_struct in question, go through the threads, then mark
> > > or bail out.  With cloning a reference to fs_struct protected by the
> > > same lock.
> >
> > Yes, this is what I meant, copy_fs() needs this lock too,
> >
> > > FWIW, I'm not at all sure that we want atomic_t for refcount in that
> > > case...
> >
> > I think you are right, because exit_fs() should take fs->lock as well.
> >
> > But, again. What whould we do when check_unsafe_exec() takes fs->lock
> > and sees that this ->fs is already marked?
> 
> Ah, I am stupid. There is no another process if this flag is set.

So...
	* one can always dereference current->fs
	* nobody can change task->fs for seen-by-scheduler task other than
current (we can, of course, do that for a task we'd just allocated in clone
before anyone else has seen it)
	* all changes of current->fs happen under task_lock *and* excl ->lock
of old value of current->fs.
	* anybody can dereference task->fs while holding task_lock(task)
	* ->lock nests inside task_lock
	* freeing happens only when the last reference is gone *and* all
tasks that used to hold such references has already gone through task_unlock
	* all refcount changes happen under excl ->lock
	* changes of ->root and ->pwd happen under excl ->lock
	* read access to ->root and ->pwd should be done under shared ->lock;
to use the contents past the unlock you need to grab references to path in
question while holding lock
	* cloning a reference is done while holding ->lock shared, fails with
-EAGAIN if fs_struct is marked "under exec"
	* mark in question is set and cleared with ->lock excl.
	* check_unsafe_exec() locks current->fs shared, goes through all
threads comparing their ->fs with our, if the number doesn't match - bail
out.  Otherwise we mark it "under exec".
	* in the end of execve() (failure or success) we clear the mark.
	* all reassignments of task->fs are either to NULL or to new instance
(never seen by anybody) or to &init_fs.
	* task with ->fs == &init_fs may not call execve(); those are kernel
threads and they must get ->fs unshared before they can do anything of that
kind (otherwise we'd have no end of trouble with chdir() done by exec'ed
binary affecting hell knows what else).

Does anybody see any holes in the above?