On Fri, Aug 07, 2015 at 02:09:35PM -0300, Herton R. Krzesinski wrote: > The current semaphore code allows a potential use after free: in exit_sem we > may > free the task's sem_undo_list while there is still another task looping > through > the same semaphore set and cleaning the sem_undo list at freeary function (the > task called IPC_RMID for the same semaphore set). > > For example, with a test program [1] running which keeps forking a lot of > processes > (which then do a semop call with SEM_UNDO flag), and with the parent right > after > removing the semaphore set with IPC_RMID, and a kernel built with CONFIG_SLAB, > CONFIG_SLAB_DEBUG and CONFIG_DEBUG_SPINLOCK, you can easily see something like > the following in the kernel log: (snip)
> Signed-off-by: Herton R. Krzesinski <[email protected]> > --- > ipc/sem.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/ipc/sem.c b/ipc/sem.c > index bc3d530..35ccddd 100644 > --- a/ipc/sem.c > +++ b/ipc/sem.c > @@ -2074,17 +2074,24 @@ void exit_sem(struct task_struct *tsk) > rcu_read_lock(); > un = list_entry_rcu(ulp->list_proc.next, > struct sem_undo, list_proc); > - if (&un->list_proc == &ulp->list_proc) > - semid = -1; > - else > - semid = un->semid; > + if (&un->list_proc == &ulp->list_proc) { > + rcu_read_unlock(); > + /* Make sure we wait for any place still referencing > + * the current ulp to finish */ > + synchronize_rcu(); > + break; > + } > + spin_lock(&ulp->lock); > + semid = un->semid; > + spin_unlock(&ulp->lock); > > + /* exit_sem raced with IPC_RMID, nothing to do */ > if (semid == -1) { > rcu_read_unlock(); > - break; > + continue; > } > > - sma = sem_obtain_object_check(tsk->nsproxy->ipc_ns, un->semid); > + sma = sem_obtain_object_check(tsk->nsproxy->ipc_ns, semid); > /* exit_sem raced with IPC_RMID, nothing to do */ > if (IS_ERR(sma)) { > rcu_read_unlock(); > @@ -2112,9 +2119,10 @@ void exit_sem(struct task_struct *tsk) > ipc_assert_locked_object(&sma->sem_perm); > list_del(&un->list_id); > > - spin_lock(&ulp->lock); > + /* we should be the last process using this ulp, so no need > + * to acquire ulp->lock here; we are also protected against > + * IPC_RMID as we hold sma->sem_perm.lock */ > list_del_rcu(&un->list_proc); > - spin_unlock(&ulp->lock); > > /* perform adjustments registered in un */ > for (i = 0; i < sma->sem_nsems; i++) { I was debugging the same issue and can confirm this fix works and makes sense. Acked-by: Aristeu Rozanski <[email protected]> -- Aristeu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
