Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1946209AbbHGTbA (ORCPT ); Fri, 7 Aug 2015 15:31:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58924 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751641AbbHGTa5 (ORCPT ); Fri, 7 Aug 2015 15:30:57 -0400 Date: Fri, 7 Aug 2015 15:30:55 -0400 From: Aristeu Rozanski To: "Herton R. Krzesinski" Cc: linux-kernel@vger.kernel.org, Andrew Morton , Davidlohr Bueso , Manfred Spraul , Rafael Aquini , Joe Perches , djeffery@redhat.com Subject: Re: [PATCH] ipc,sem: fix use after free on IPC_RMID after a task using same semaphore set exits Message-ID: <20150807193055.GD23305@redhat.com> References: <1438967375-14877-1-git-send-email-herton@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1438967375-14877-1-git-send-email-herton@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2862 Lines: 80 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 > --- > 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 -- Aristeu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/