[PATCH] close race condition in shared memory mapping/unmapping

[PATCH] close race condition in shared memory mapping/unmapping

[Neil Horman]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

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
-- 
/***************************************************
  *Neil Horman
  *Software Engineer
  *Red Hat, Inc.
  *nhorman@redhat.com
  *gpg keyid: 1024D / 0x92A74FA1
  *http://pgp.mit.edu
  ***************************************************/

[-- Attachment #2: linux-2.6.9-rc2-shmlock.patch --]
[-- Type: text/plain, Size: 1325 bytes --]

--- linux-2.6.9-rc2-rpc/ipc/shm.c.orig	2004-09-13 15:59:46.604446096 -0400
+++ linux-2.6.9-rc2-rpc/ipc/shm.c	2004-09-13 16:17:05.606493776 -0400
@@ -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)) {

Re: [PATCH] close race condition in shared memory mapping/unmapping

[Felipe W Damasio]

	Hi Neil,

Neil Horman wrote:
> +	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

Re: [PATCH] close race condition in shared memory mapping/unmapping

[Neil Horman]

Felipe W Damasio wrote:
>     Hi Neil,
> 
> Neil Horman wrote:
> 
>> +    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
Crap, good catch.  I'll repost with that fixed.  Anything else you see a 
problem with?
Regards
Neil

-- 
/***************************************************
  *Neil Horman
  *Software Engineer
  *Red Hat, Inc.
  *nhorman@redhat.com
  *gpg keyid: 1024D / 0x92A74FA1
  *http://pgp.mit.edu
  ***************************************************/

Re: [PATCH] close race condition in shared memory mapping/unmapping

[Chris Wright]

* Neil Horman (nhorman@redhat.com) 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
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] close race condition in shared memory mapping/unmapping

[Neil Horman]

Chris Wright wrote:
> * Neil Horman (nhorman@redhat.com) 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
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

-- 
/***************************************************
  *Neil Horman
  *Software Engineer
  *Red Hat, Inc.
  *nhorman@redhat.com
  *gpg keyid: 1024D / 0x92A74FA1
  *http://pgp.mit.edu
  ***************************************************/