Skip to contentSkip to conversation

close race condition in shared memory mapping/unmapping

PATCH In review Neil Horman 3 participants 5 messages Sep 13, 2004 – Sep 14, 2004

lore
Share & tools
lore

Neil Horman

author
#0 lore raw

PATCH 1/1 close race condition in shared memory mapping/unmapping

Hey all-
Found this the other day poking through the ipc code. There appears to
be a race condition in the counter that records how many processes are accessing a given shared memory segment. In most places the shm_nattch variable is protected by the shm_ids.sem semaphore, but there are a few openings which appear to be able to allow a corruption of this variable when run on SMP systems. I've attached a patch to 2.6.9-rc2 for review.
The locking may be a little over-aggressive (I was following examples
from other points in this file), but I figure better safe than sorry :).

Anywho, here it is.
Thanks
Neil

Signature · 8 lines
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *[email protected]
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/
ipc/shm.c renamed from ipc/shm.c.orig +9 -0
@@ -86,12 +86,14 @@
 static inline void shm_inc (int id) {
 	struct shmid_kernel *shp;
 
+	down(&shm_ids.sem);
 	if(!(shp = shm_lock(id)))
 		BUG();
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = current->tgid;
 	shp->shm_nattch++;
 	shm_unlock(shp);
+	up(&shm_ids.sem);
 }
 
 /* This is called by fork, once for every shm attach. */

@@ -697,18 +699,23 @@
 	 * We cannot rely on the fs check since SYSV IPC does have an
 	 * additional creator id...
 	 */
+	down(&shm_ids.sem);
 	shp = shm_lock(shmid);
 	if(shp == NULL) {
+		shm_unlock(shp);
+		up(&shm_ids.sem);
 		err = -EINVAL;
 		goto out;
 	}
 	err = shm_checkid(shp,shmid);
 	if (err) {
 		shm_unlock(shp);
+		up(&shm_ids.sem);
 		goto out;
 	}
 	if (ipcperms(&shp->shm_perm, acc_mode)) {
 		shm_unlock(shp);
+		up(&shm_ids.sem);
 		err = -EACCES;
 		goto out;
 	}

@@ -716,6 +723,7 @@
 	err = security_shm_shmat(shp, shmaddr, shmflg);
 	if (err) {
 		shm_unlock(shp);
+		up(&shm_ids.sem);
 		return err;
 	}
 		

@@ -723,6 +731,7 @@
 	size = i_size_read(file->f_dentry->d_inode);
 	shp->shm_nattch++;
 	shm_unlock(shp);
+	up(&shm_ids.sem);
 
 	down_write(&current->mm->mmap_sem);
 	if (addr && !(shmflg & SHM_REMAP)) {
#1 lore raw

Hi Neil,

ipc/shm.c line 709

Quoted 8 lines · Neil Horman
ipc/shm.c renamed from ipc/shm.c.orig +3 -0
+	down(&shm_ids.sem);
 	shp = shm_lock(shmid);
 	if(shp == NULL) {
+		shm_unlock(shp);
+		up(&shm_ids.sem);
 		err = -EINVAL;
 		goto out;
 	}

Trying to unlock a NULL pointer?

Cheers,

Felipe

author
#2 lore raw
Quoted 14 lines · Felipe W Damasio

Hi Neil,

ipc/shm.c line 709

Quoted 8 lines · Neil Horman
ipc/shm.c renamed from ipc/shm.c.orig +3 -0
+    down(&shm_ids.sem);
     shp = shm_lock(shmid);
     if(shp == NULL) {
+        shm_unlock(shp);
+        up(&shm_ids.sem);
         err = -EINVAL;
         goto out;
     }
Quoted 5 lines

Trying to unlock a NULL pointer?

Cheers,

Felipe

Crap, good catch. I'll repost with that fixed. Anything else you see a problem with? Regards
Neil

Signature · 8 lines
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *[email protected]
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/
#3 lore raw
In reply to Neil Horman
Quoted 9 lines · * Neil Horman ([email protected]) wrote:

Hey all-
Found this the other day poking through the ipc code. There appears to
be a race condition in the counter that records how many processes are accessing a given shared memory segment. In most places the shm_nattch variable is protected by the shm_ids.sem semaphore, but there are a few openings which appear to be able to allow a corruption of this variable when run on SMP systems. I've attached a patch to 2.6.9-rc2 for review.
The locking may be a little over-aggressive (I was following examples
from other points in this file), but I figure better safe than sorry :).

Are you sure you've got this right? I thought that the shmid_kernel struct protects shm_nattch with a local (per structure) lock which is embedded in kern_ipc_perm. Did you find shm_nattch changes w/out shm_lock/shm_unlock around it? I believe shm_ids.sem is protecting the id allocation, not per object data such as shm_nattch.

thanks,
-chris

Signature · 1 line
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net
author
#4 lore raw
Quoted 9 lines · Chris Wright
Quoted 5 lines · * Neil Horman ([email protected]) wrote:

Hey all-
Found this the other day poking through the ipc code. There appears to
be a race condition in the counter that records how many processes are accessing a given shared memory segment. In most places the shm_nattch variable is protected by the shm_ids.sem semaphore, but there are a few openings which appear to be able to allow a corruption of this variable when run on SMP systems. I've attached a patch to 2.6.9-rc2 for review.
The locking may be a little over-aggressive (I was following examples
from other points in this file), but I figure better safe than sorry :).

Quoted 8 lines

Are you sure you've got this right? I thought that the shmid_kernel struct protects shm_nattch with a local (per structure) lock which is embedded in kern_ipc_perm. Did you find shm_nattch changes w/out shm_lock/shm_unlock around it? I believe shm_ids.sem is protecting the id allocation, not per object data such as shm_nattch.

thanks,
-chris

You're right, its not correct. I'm sorry. I'm looking into a locking bug in 2.4, which does its ipc locking for shared memory very differently. Since the shared memory code looks simmilar in 2.6, I was making the assumption that the change applied upstram as well, but I don't think thats the case after looking more carefully. My bad. Neil

Signature · 8 lines
/***************************************************
 *Neil Horman
 *Software Engineer
 *Red Hat, Inc.
 *[email protected]
 *gpg keyid: 1024D / 0x92A74FA1
 *http://pgp.mit.edu
 ***************************************************/