linux-kernel - [Patch] mqueue: fix the bad code in sys_mq

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100225134023.GB3842@hack>
Date:	Thu, 25 Feb 2010 21:40:23 +0800
From:	Américo Wang <xiyou.wangcong@...il.com>
To:	LKML <linux-kernel@...r.kernel.org>
Cc:	Américo Wang <xiyou.wangcong@...il.com>,
	André Goddard Rosa <andre.goddard@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Serge E . Hallyn" <serue@...ibm.com>,
	Cedric Le Goater <clg@...ibm.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Xiaotian Feng <xtfeng@...il.com>
Subject: [Patch] mqueue: fix the bad code in sys_mq_open()


Fix bad code in ipc/mqueue.c.

Inspired by André Goddard Rosa's  patch.

a) do_create() and do_open() should not release the resources which
   are not acquired within themselves, it's their caller's work;

b) Fix an fd leak;

c) The goto label 'out_upsem' doesn't make any sense, rename to
   'out_unlock';

d) mntget() should be called before looking up dentry within
   ->mnt->mnt_root;

e) When dealing with failure case, we should release the resources
   in a reversed order of acquiring, so reorder the goto labels;

f) Remove some unused labels due to reorder.

Also, this shrinks the binary by 60 bytes:

   text	      data     bss       dec	    hex	filename
   7674       1684        8      9366      2496	ipc/mqueue.o.BEFORE
   7614	      1684        8      9306      245a	ipc/mqueue.o.AFTER


Signed-off-by: WANG Cong <xiyou.wangcong@...il.com>
Cc: André Goddard Rosa <andre.goddard@...il.com>

---
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index c79bd57..fdd09da 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -650,8 +650,6 @@ static struct file *do_create(struct ipc_namespace *ipc_ns, struct dentry *dir,
 out_drop_write:
 	mnt_drop_write(ipc_ns->mq_mnt);
 out:
-	dput(dentry);
-	mntput(ipc_ns->mq_mnt);
 	return ERR_PTR(ret);
 }
 
@@ -664,17 +662,11 @@ static struct file *do_open(struct ipc_namespace *ipc_ns,
 	static const int oflag2acc[O_ACCMODE] = { MAY_READ, MAY_WRITE,
 						  MAY_READ | MAY_WRITE };
 
-	if ((oflag & O_ACCMODE) == (O_RDWR | O_WRONLY)) {
-		dput(dentry);
-		mntput(ipc_ns->mq_mnt);
+	if ((oflag & O_ACCMODE) == (O_RDWR | O_WRONLY))
 		return ERR_PTR(-EINVAL);
-	}
 
-	if (inode_permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE])) {
-		dput(dentry);
-		mntput(ipc_ns->mq_mnt);
+	if (inode_permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE]))
 		return ERR_PTR(-EACCES);
-	}
 
 	return dentry_open(dentry, ipc_ns->mq_mnt, oflag, cred);
 }
@@ -686,7 +678,7 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
 	struct file *filp;
 	char *name;
 	struct mq_attr attr;
-	int fd, error;
+	int fd, error = 0;
 	struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
 
 	if (u_attr && copy_from_user(&attr, u_attr, sizeof(struct mq_attr)))
@@ -701,13 +693,13 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
 	if (fd < 0)
 		goto out_putname;
 
+	mntget(ipc_ns->mq_mnt);
 	mutex_lock(&ipc_ns->mq_mnt->mnt_root->d_inode->i_mutex);
 	dentry = lookup_one_len(name, ipc_ns->mq_mnt->mnt_root, strlen(name));
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);
-		goto out_err;
+		goto out_unlock;
 	}
-	mntget(ipc_ns->mq_mnt);
 
 	if (oflag & O_CREAT) {
 		if (dentry->d_inode) {	/* entry already exists */
@@ -731,24 +723,23 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
 
 	if (IS_ERR(filp)) {
 		error = PTR_ERR(filp);
-		goto out_putfd;
+		goto out;
 	}
 
 	fd_install(fd, filp);
-	goto out_upsem;
+	goto out_unlock;
 
 out:
 	dput(dentry);
-	mntput(ipc_ns->mq_mnt);
-out_putfd:
-	put_unused_fd(fd);
-out_err:
-	fd = error;
-out_upsem:
+out_unlock:
 	mutex_unlock(&ipc_ns->mq_mnt->mnt_root->d_inode->i_mutex);
+	if (error) {
+		mntput(ipc_ns->mq_mnt);
+		put_unused_fd(fd);
+	}
 out_putname:
 	putname(name);
-	return fd;
+	return error;
 }
 
 SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
--
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/