--- 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(¤t->mm->mmap_sem);
if (addr && !(shmflg & SHM_REMAP)) {
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
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.
*[email protected]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***************************************************/
* 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
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Chris Wright wrote:
> * 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
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.
*[email protected]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***************************************************/