| 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: <1224827972.2595.107.camel@frecb000730.frec.bull.fr>
Date: Fri, 24 Oct 2008 07:59:32 +0200
From: Nadia Derbey <Nadia.Derbey@...l.net>
To: akpm@...ux-foundation.org
Cc: cboulte@...il.com, manfred@...orfullife.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] SYSVIPC - Fix the ipc structures initialization
On Thu, 2008-10-23 at 15:57 +0200, Nadia.Derbey@...l.net wrote:
> plain text document attachment
> (ipc-fix-sysvipc_structures_initialization.patch)
> This patch is a fix for Bugzilla bug
> http://bugzilla.kernel.org/show_bug.cgi?id=11796.
>
> To summarize, a simple testcase is concurrently running an infinite loop on
> msgctl(IPC_STAT) and a call to msgget():
>
> while (1)
> msgctl(id, IPC_STAT) 1
> |
> |
> |
> 2 id = msgget(key, IPC_CREAT)
> |
> |
> |
>
> In the interval [1-2], the id doesn't exist yet.
>
> In that test, the problem is the following:
> When we are calling ipc_addid() from msgget() the msq structure is not
> completely initialized. So idr_get_new() is inserting a pointer into the
> idr tree, and the structure which is pointed to has, among other fields,
> its lock uninitialized.
>
> Since msgctl(IPC_STAT) is looping while (1), idr_find() returns the
> pointer as soon as it is inserted into the IDR tree. And ipc_lock()
> calls spin_lock(&mqs->lock), while we have not initialized that lock
> yet.
>
> This patch moves the spin_lock_init() before the call to ipc_addid().
> It also sets the "deleted" flag to 1 in the window between msg structure
> allocation and msg structure locking in ipc_addid().
>
> The fix has been applied to semaphores and shm segments too.
I just did my own review :-) and found out there's a possible
factorization: the 'deleted' flag can be set once in util.c, instead of
setting it separately for msg queues, semaphores and shm.
Resending the patch right now. Sorry for the noise.
Regards,
Nadia
>
> Regards,
> Nadia
>
>
> Signed-off-by: Nadia Derbey <Nadia.Derbey@...l.net>
>
> ---
> ipc/msg.c | 9 +++++++++
> ipc/sem.c | 9 +++++++++
> ipc/shm.c | 9 +++++++++
> ipc/util.c | 3 ++-
> 4 files changed, 29 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.27/ipc/msg.c
> ===================================================================
> --- linux-2.6.27.orig/ipc/msg.c 2008-10-23 15:20:46.000000000 +0200
> +++ linux-2.6.27/ipc/msg.c 2008-10-23 16:38:59.000000000 +0200
> @@ -191,6 +191,15 @@ static int newque(struct ipc_namespace *
> msq->q_perm.mode = msgflg & S_IRWXUGO;
> msq->q_perm.key = key;
>
> + /*
> + * We have a window between the time msq is inserted into the idr
> + * tree (done in ipc_addid()) and the time it is actually locked.
> + * In order to be safe during that window set the msq as deleted:
> + * a concurrent ipc_lock() will see it as not present until the
> + * initialization phase is complete.
> + */
> + msq->q_perm.deleted = 1;
> +
> msq->q_perm.security = NULL;
> retval = security_msg_queue_alloc(msq);
> if (retval) {
> Index: linux-2.6.27/ipc/util.c
> ===================================================================
> --- linux-2.6.27.orig/ipc/util.c 2008-10-23 15:20:46.000000000 +0200
> +++ linux-2.6.27/ipc/util.c 2008-10-23 15:33:37.000000000 +0200
> @@ -266,6 +266,8 @@ int ipc_addid(struct ipc_ids* ids, struc
> if (ids->in_use >= size)
> return -ENOSPC;
>
> + spin_lock_init(&new->lock);
> +
> err = idr_get_new(&ids->ipcs_idr, new, &id);
> if (err)
> return err;
> @@ -280,7 +282,6 @@ int ipc_addid(struct ipc_ids* ids, struc
> ids->seq = 0;
>
> new->id = ipc_buildid(id, new->seq);
> - spin_lock_init(&new->lock);
> new->deleted = 0;
> rcu_read_lock();
> spin_lock(&new->lock);
> Index: linux-2.6.27/ipc/sem.c
> ===================================================================
> --- linux-2.6.27.orig/ipc/sem.c 2008-10-23 15:20:46.000000000 +0200
> +++ linux-2.6.27/ipc/sem.c 2008-10-23 15:57:27.000000000 +0200
> @@ -256,6 +256,15 @@ static int newary(struct ipc_namespace *
> sma->sem_perm.mode = (semflg & S_IRWXUGO);
> sma->sem_perm.key = key;
>
> + /*
> + * We have a window between the time sma is inserted into the idr
> + * tree (done in ipc_addid()) and the time it is actually locked.
> + * In order to be safe during that window set the sma as deleted:
> + * a concurrent ipc_lock() will see it as not present until the
> + * initialization phase is complete.
> + */
> + sma->sem_perm.deleted = 1;
> +
> sma->sem_perm.security = NULL;
> retval = security_sem_alloc(sma);
> if (retval) {
> Index: linux-2.6.27/ipc/shm.c
> ===================================================================
> --- linux-2.6.27.orig/ipc/shm.c 2008-10-23 15:20:46.000000000 +0200
> +++ linux-2.6.27/ipc/shm.c 2008-10-23 15:56:32.000000000 +0200
> @@ -355,6 +355,15 @@ static int newseg(struct ipc_namespace *
> shp->shm_perm.mode = (shmflg & S_IRWXUGO);
> shp->mlock_user = NULL;
>
> + /*
> + * We have a window between the time shp is inserted into the idr
> + * tree (done in ipc_addid()) and the time it is actually locked.
> + * In order to be safe during that window set the shp as deleted:
> + * a concurrent ipc_lock() will see it as not present until the
> + * initialization phase is complete.
> + */
> + shp->shm_perm.deleted = 1;
> +
> shp->shm_perm.security = NULL;
> error = security_shm_alloc(shp);
> if (error) {
>
--
Nadia Derbey <Nadia.Derbey@...l.net>
--
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