[PATCH] fix memleak in sys_mq_timedsend

[PATCH] fix memleak in sys_mq_timedsend

[Chris Wright]

Move error handling to capture all three possible error conditions on
sending to a full queue.  Without this fix any unprivileged user can
leak arbitrary amounts of kernel memory.

--- ./ipc/mqueue.c~fix_memleak	2004-05-04 15:08:52.000000000 -0700
+++ ./ipc/mqueue.c	2004-05-04 15:10:59.000000000 -0700
@@ -811,9 +811,9 @@ asmlinkage long sys_mq_timedsend(mqd_t m
 			wait.msg = (void *) msg_ptr;
 			wait.state = STATE_NONE;
 			ret = wq_sleep(info, SEND, timeout, &wait);
-			if (ret < 0)
-				free_msg(msg_ptr);
 		}
+		if (ret < 0)
+			free_msg(msg_ptr);
 	} else {
 		receiver = wq_get_first_waiter(info, RECV);
 		if (receiver) {

[PATCH] fix queues_count accounting in mqueue_delete_inode()

[Chris Wright]

During mqueue_get_inode(), it's possible that kmalloc() of the
info->messages array will fail.  This failure mode will cause the
queues_count to be (incorrectly) decremented twice.  This patch uses
info->messages on mqueue_delete_inode() to determine whether the
mqueue was every truly created, and hence proper accounting is needed
on destruction.

--- ./ipc/mqueue.c~fix_queues_count	2004-05-04 15:10:59.000000000 -0700
+++ ./ipc/mqueue.c	2004-05-04 15:16:34.000000000 -0700
@@ -215,9 +215,11 @@ static void mqueue_delete_inode(struct i
 
 	clear_inode(inode);
 
-	spin_lock(&mq_lock);
-	queues_count--;
-	spin_unlock(&mq_lock);
+	if (info->messages) {
+		spin_lock(&mq_lock);
+		queues_count--;
+		spin_unlock(&mq_lock);
+	}
 }
 
 static int mqueue_create(struct inode *dir, struct dentry *dentry,

[PATCH] simplify mqueue_inode_info->messages allocation

[Chris Wright]

Currently, if a user creates an mqueue and passes an mq_attr, the
info->messages will be created twice (and the extra one is properly
freed).  This patch simply delays the allocation so that it only ever
happens once.  The relevant mq_attr data is passed to lower levels via
the dentry->d_fsdata fs private data.  This also helps isolate the areas
we'd need to touch to do rlimits on mqueues.

--- ./ipc/mqueue.c~single_alloc	2004-05-04 15:16:34.000000000 -0700
+++ ./ipc/mqueue.c~	2004-05-04 15:59:25.000000000 -0700
@@ -97,7 +97,8 @@ static inline struct mqueue_inode_info *
 	return container_of(inode, struct mqueue_inode_info, vfs_inode);
 }
 
-static struct inode *mqueue_get_inode(struct super_block *sb, int mode)
+static struct inode *mqueue_get_inode(struct super_block *sb, int mode,
+							struct mq_attr *attr)
 {
 	struct inode *inode;
 
@@ -127,7 +128,11 @@ static struct inode *mqueue_get_inode(st
 			memset(&info->attr, 0, sizeof(info->attr));
 			info->attr.mq_maxmsg = DFLT_MSGMAX;
 			info->attr.mq_msgsize = DFLT_MSGSIZEMAX;
-			info->messages = kmalloc(DFLT_MSGMAX * sizeof(struct msg_msg *), GFP_KERNEL);
+			if (attr) {
+				info->attr.mq_maxmsg = attr->mq_maxmsg;
+				info->attr.mq_msgsize = attr->mq_msgsize;
+			}
+			info->messages = kmalloc(info->attr.mq_maxmsg * sizeof(struct msg_msg *), GFP_KERNEL);
 			if (!info->messages) {
 				make_bad_inode(inode);
 				iput(inode);
@@ -153,7 +158,7 @@ static int mqueue_fill_super(struct supe
 	sb->s_magic = MQUEUE_MAGIC;
 	sb->s_op = &mqueue_super_ops;
 
-	inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO);
+	inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO, NULL);
 	if (!inode)
 		return -ENOMEM;
 
@@ -226,6 +231,7 @@ static int mqueue_create(struct inode *d
 				int mode, struct nameidata *nd)
 {
 	struct inode *inode;
+	struct mq_attr *attr = dentry->d_fsdata;
 	int error;
 
 	spin_lock(&mq_lock);
@@ -236,7 +242,7 @@ static int mqueue_create(struct inode *d
 	queues_count++;
 	spin_unlock(&mq_lock);
 
-	inode = mqueue_get_inode(dir->i_sb, mode);
+	inode = mqueue_get_inode(dir->i_sb, mode, attr);
 	if (!inode) {
 		error = -ENOMEM;
 		spin_lock(&mq_lock);
@@ -535,9 +541,6 @@ static struct file *do_create(struct den
 			int oflag, mode_t mode, struct mq_attr __user *u_attr)
 {
 	struct file *filp;
-	struct inode *inode;
-	struct mqueue_inode_info *info;
-	struct msg_msg **msgs = NULL;
 	struct mq_attr attr;
 	int ret;
 
@@ -555,28 +558,14 @@ static struct file *do_create(struct den
 					attr.mq_msgsize > msgsize_max)
 				return ERR_PTR(-EINVAL);
 		}
-		msgs = kmalloc(attr.mq_maxmsg * sizeof(*msgs), GFP_KERNEL);
-		if (!msgs)
-			return ERR_PTR(-ENOMEM);
-	} else {
-		msgs = NULL;
+		/* store for use during create */
+		dentry->d_fsdata = &attr;
 	}
 
 	ret = vfs_create(dir->d_inode, dentry, mode, NULL);
-	if (ret) {
-		kfree(msgs);
+	dentry->d_fsdata = NULL;
+	if (ret)
 		return ERR_PTR(ret);
-	}
-
-	inode = dentry->d_inode;
-	info = MQUEUE_I(inode);
-
-	if (msgs) {
-		info->attr.mq_maxmsg = attr.mq_maxmsg;
-		info->attr.mq_msgsize = attr.mq_msgsize;
-		kfree(info->messages);
-		info->messages = msgs;
-	}
 
 	filp = dentry_open(dentry, mqueue_mnt, oflag);
 	if (!IS_ERR(filp))

Re: [PATCH] simplify mqueue_inode_info->messages allocation

[Chris Wright]

* Chris Wright (chrisw@osdl.org) wrote:
> --- ./ipc/mqueue.c~single_alloc	2004-05-04 15:16:34.000000000 -0700
> +++ ./ipc/mqueue.c~	2004-05-04 15:59:25.000000000 -0700

Ugh!  Andrew pointed out to me that this is crap.  Sorry about the added
noise.  Here's a patch relative to a file that actually exists.

===== ipc/mqueue.c 1.9 vs edited =====
--- 1.9/ipc/mqueue.c	Sat Apr 17 11:19:31 2004
+++ edited/ipc/mqueue.c	Tue May  4 18:28:26 2004
@@ -97,7 +97,8 @@
 	return container_of(inode, struct mqueue_inode_info, vfs_inode);
 }
 
-static struct inode *mqueue_get_inode(struct super_block *sb, int mode)
+static struct inode *mqueue_get_inode(struct super_block *sb, int mode,
+							struct mq_attr *attr)
 {
 	struct inode *inode;
 
@@ -127,7 +128,11 @@
 			memset(&info->attr, 0, sizeof(info->attr));
 			info->attr.mq_maxmsg = DFLT_MSGMAX;
 			info->attr.mq_msgsize = DFLT_MSGSIZEMAX;
-			info->messages = kmalloc(DFLT_MSGMAX * sizeof(struct msg_msg *), GFP_KERNEL);
+			if (attr) {
+				info->attr.mq_maxmsg = attr->mq_maxmsg;
+				info->attr.mq_msgsize = attr->mq_msgsize;
+			}
+			info->messages = kmalloc(info->attr.mq_maxmsg * sizeof(struct msg_msg *), GFP_KERNEL);
 			if (!info->messages) {
 				make_bad_inode(inode);
 				iput(inode);
@@ -153,7 +158,7 @@
 	sb->s_magic = MQUEUE_MAGIC;
 	sb->s_op = &mqueue_super_ops;
 
-	inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO);
+	inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO, NULL);
 	if (!inode)
 		return -ENOMEM;
 
@@ -224,6 +229,7 @@
 				int mode, struct nameidata *nd)
 {
 	struct inode *inode;
+	struct mq_attr *attr = dentry->d_fsdata;
 	int error;
 
 	spin_lock(&mq_lock);
@@ -234,7 +240,7 @@
 	queues_count++;
 	spin_unlock(&mq_lock);
 
-	inode = mqueue_get_inode(dir->i_sb, mode);
+	inode = mqueue_get_inode(dir->i_sb, mode, attr);
 	if (!inode) {
 		error = -ENOMEM;
 		spin_lock(&mq_lock);
@@ -533,9 +539,6 @@
 			int oflag, mode_t mode, struct mq_attr __user *u_attr)
 {
 	struct file *filp;
-	struct inode *inode;
-	struct mqueue_inode_info *info;
-	struct msg_msg **msgs = NULL;
 	struct mq_attr attr;
 	int ret;
 
@@ -553,28 +556,14 @@
 					attr.mq_msgsize > msgsize_max)
 				return ERR_PTR(-EINVAL);
 		}
-		msgs = kmalloc(attr.mq_maxmsg * sizeof(*msgs), GFP_KERNEL);
-		if (!msgs)
-			return ERR_PTR(-ENOMEM);
-	} else {
-		msgs = NULL;
+		/* store for use during create */
+		dentry->d_fsdata = &attr;
 	}
 
 	ret = vfs_create(dir->d_inode, dentry, mode, NULL);
-	if (ret) {
-		kfree(msgs);
+	dentry->d_fsdata = NULL;
+	if (ret)
 		return ERR_PTR(ret);
-	}
-
-	inode = dentry->d_inode;
-	info = MQUEUE_I(inode);
-
-	if (msgs) {
-		info->attr.mq_maxmsg = attr.mq_maxmsg;
-		info->attr.mq_msgsize = attr.mq_msgsize;
-		kfree(info->messages);
-		info->messages = msgs;
-	}
 
 	filp = dentry_open(dentry, mqueue_mnt, oflag);
 	if (!IS_ERR(filp))

Re: [PATCH] simplify mqueue_inode_info->messages allocation

[Marcelo Tosatti]

On Tue, May 04, 2004 at 06:37:22PM -0700, Chris Wright wrote:
> * Chris Wright (chrisw@osdl.org) wrote:
> > --- ./ipc/mqueue.c~single_alloc	2004-05-04 15:16:34.000000000 -0700
> > +++ ./ipc/mqueue.c~	2004-05-04 15:59:25.000000000 -0700
> 
> Ugh!  Andrew pointed out to me that this is crap.  Sorry about the added
> noise.  Here's a patch relative to a file that actually exists. 

While we're at it, in do_create:

        ret = vfs_create(dir->d_inode, dentry, mode, NULL);
        if (ret) {
                kfree(msgs);
                return ERR_PTR(ret);

The msgs pointer can be NULL. Isnt that going to BUG if so?

Re: [PATCH] simplify mqueue_inode_info->messages allocation

[Chris Wright]

* Marcelo Tosatti (marcelo.tosatti@cyclades.com) wrote:
> On Tue, May 04, 2004 at 06:37:22PM -0700, Chris Wright wrote:
> > * Chris Wright (chrisw@osdl.org) wrote:
> > > --- ./ipc/mqueue.c~single_alloc	2004-05-04 15:16:34.000000000 -0700
> > > +++ ./ipc/mqueue.c~	2004-05-04 15:59:25.000000000 -0700
> > 
> > Ugh!  Andrew pointed out to me that this is crap.  Sorry about the added
> > noise.  Here's a patch relative to a file that actually exists. 
> 
> While we're at it, in do_create:
> 
>         ret = vfs_create(dir->d_inode, dentry, mode, NULL);
>         if (ret) {
>                 kfree(msgs);
>                 return ERR_PTR(ret);
> 
> The msgs pointer can be NULL. Isnt that going to BUG if so?

kfree(NULL) is no-op.  but if you look at this patch, that is gone
altogether.

thanks,
-chris

-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net