| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20131217214653.GC31827@localhost.localdomain>
Date: Tue, 17 Dec 2013 19:46:54 -0200
From: Rafael Aquini <aquini@...hat.com>
To: Davidlohr Bueso <davidlohr@...com>
Cc: linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Rik van Riel <riel@...hat.com>,
Greg Thelen <gthelen@...gle.com>,
Manfred Spraul <manfred@...orfullife.com>
Subject: Re: [PATCH] ipc: introduce ipc_valid_object() helper to sort out
IPC_RMID races
On Tue, Dec 17, 2013 at 01:27:49PM -0800, Davidlohr Bueso wrote:
> Ccing Manfred.
>
> On Tue, 2013-12-17 at 17:03 -0200, Rafael Aquini wrote:
> > After the locking semantics for the SysV IPC API got improved, a couple of
> > IPC_RMID race windows were opened because we ended up dropping the
> > 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > The spotted races got sorted out by re-introducing the old test within
> > the racy critical sections.
> >
> > This patch introduces ipc_valid_object() to consolidate the way we cope with
> > IPC_RMID races by using the same abstraction across the API implementation.
>
> This is certainly a good function to have. Some comments below.
>
> >
> > Signed-off-by: Rafael Aquini <aquini@...hat.com>
> > ---
> > ipc/msg.c | 7 ++++---
> > ipc/sem.c | 8 ++++----
> > ipc/shm.c | 16 ++++++++--------
> > ipc/util.h | 13 +++++++++++++
> > 4 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/ipc/msg.c b/ipc/msg.c
> > index 558aa91..8983ea5 100644
> > --- a/ipc/msg.c
> > +++ b/ipc/msg.c
> > @@ -696,7 +696,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> > goto out_unlock0;
> >
> > /* raced with RMID? */
> > - if (msq->q_perm.deleted) {
> > + if (!ipc_valid_object(&msq->q_perm)) {
> > err = -EIDRM;
> > goto out_unlock0;
> > }
> > @@ -731,7 +731,8 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> > ipc_lock_object(&msq->q_perm);
> >
> > ipc_rcu_putref(msq, ipc_rcu_free);
> > - if (msq->q_perm.deleted) {
> > + /* raced with RMID? */
> > + if (!ipc_valid_object(&msq->q_perm)) {
> > err = -EIDRM;
> > goto out_unlock0;
> > }
> > @@ -909,7 +910,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
> > ipc_lock_object(&msq->q_perm);
> >
> > /* raced with RMID? */
> > - if (msq->q_perm.deleted) {
> > + if (!ipc_valid_object(&msq->q_perm)) {
> > msg = ERR_PTR(-EIDRM);
> > goto out_unlock0;
> > }
> > diff --git a/ipc/sem.c b/ipc/sem.c
> > index db9d241..f4fad32 100644
> > --- a/ipc/sem.c
> > +++ b/ipc/sem.c
> > @@ -1282,7 +1282,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
> >
> > sem_lock(sma, NULL, -1);
> >
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > sem_unlock(sma, -1);
> > rcu_read_unlock();
> > return -EIDRM;
> > @@ -1342,7 +1342,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> > int i;
> >
> > sem_lock(sma, NULL, -1);
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > err = -EIDRM;
> > goto out_unlock;
> > }
> > @@ -1435,7 +1435,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> > goto out_rcu_wakeup;
> >
> > sem_lock(sma, NULL, -1);
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > err = -EIDRM;
> > goto out_unlock;
> > }
> > @@ -2068,7 +2068,7 @@ void exit_sem(struct task_struct *tsk)
> >
> > sem_lock(sma, NULL, -1);
> > /* exit_sem raced with IPC_RMID, nothing to do */
> > - if (sma->sem_perm.deleted) {
> > + if (!ipc_valid_object(&sma->sem_perm)) {
> > sem_unlock(sma, -1);
> > rcu_read_unlock();
> > continue;
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 7a51443..1bc68f1 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -975,6 +975,13 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> > goto out_unlock1;
> >
> > ipc_lock_object(&shp->shm_perm);
> > +
> > + /* check if shm_destroy() is tearing down shp */
> > + if (!ipc_valid_object(&shp->shm_perm)) {
> > + err = -EIDRM;
> > + goto out_unlock0;
> > + }
> > +
> > if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
> > kuid_t euid = current_euid();
> > if (!uid_eq(euid, shp->shm_perm.uid) &&
> > @@ -989,13 +996,6 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> > }
> >
> > shm_file = shp->shm_file;
> > -
> > - /* check if shm_destroy() is tearing down shp */
> > - if (shm_file == NULL) {
> > - err = -EIDRM;
> > - goto out_unlock0;
> > - }
>
> Ok, this seems safe, we can always rely on .deleted for validity since
> shm_destroy() ends up calling shm_rmid() which sets .deleted. So this
> change is really moving what we're checking against just a few
> instructions later.
>
Yep, I did change it cause it seems that there's no reason to delay the return
condition if we raced with shm_destroy(), anyways.
> > -
> > if (is_file_hugepages(shm_file))
> > goto out_unlock0;
> >
> > @@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> > ipc_lock_object(&shp->shm_perm);
> >
> > /* check if shm_destroy() is tearing down shp */
> > - if (shp->shm_file == NULL) {
> > + if (!ipc_valid_object(&shp->shm_perm)) {
> > ipc_unlock_object(&shp->shm_perm);
> > err = -EIDRM;
> > goto out_unlock;
> > diff --git a/ipc/util.h b/ipc/util.h
> > index 59d78aa..3a5f0d0 100644
> > --- a/ipc/util.h
> > +++ b/ipc/util.h
> > @@ -185,6 +185,19 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
> > rcu_read_unlock();
> > }
> >
> > +/*
> > + * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
> > + * where the respective ipc_ids.rwsem is not being held down.
> > + * Checks whether the ipc object is still around or if it's gone already, as
> > + * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
> > + * Needs to be called with kern_ipc_perm.lock held.
> > + */
> > +static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
> > +{
> > + assert_spin_locked(&perm->lock);
>
> This is already guaranteed, we don't need it (not that it's much of an
> overhead in any case). IMO the comment above explicitly saying to call
> it with the ipc lock held is enough.
>
Good thought. I'll drop the assert and cover one extra case in sem.c where the
check can be done lockless (just spotted it).
Will submit a V2 soon with the change above! Thanks for the review.
>
> > + return perm->deleted == 0;
>
> We should turn this into a bool (yes, that's another patch).
>
Yep, I thought doing the scalar-2-bool conversion here and submit another patch
later, for a total boolean convertion, when (if) this one gets accepted.
Thanks for your thoughts!
-- Rafael
> > +}
> > +
> > struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id);
> > int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
> > struct ipc_ops *ops, struct ipc_params *params);
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists