| 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
| ||
|
Message-ID: <b8bf37781002250741r3836a1calec3441ced7afb863@mail.gmail.com>
Date: Thu, 25 Feb 2010 12:41:47 -0300
From: André Goddard Rosa <andre.goddard@...il.com>
To: Américo Wang <xiyou.wangcong@...il.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
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: Re: [Patch] mqueue: fix the bad code in sys_mq_open()
Hi, Américo!
On Thu, Feb 25, 2010 at 10:40 AM, Américo Wang <xiyou.wangcong@...il.com> wrote:
>
> 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)
I have some questions below:
Inside do_open() and do_create() on mqueue.c, we call
dentry_open()/__dentry_open().
If dentry_open() fails, it'll automatically call:
dput(dentry);
mntput(mnt);
Then we'll go here and set "error":
> if (IS_ERR(filp)) {
> error = PTR_ERR(filp);
> - goto out_putfd;
> + goto out;
> }
>
And finally here:
> 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);
> + }
Is it ok to call:
dput(dentry);
mntput(ipc_ns->mq_mnt);
multiple times?
I also do not see where you set "error" to "fd":
> - return fd;
> + return error;
Am I missing something?
Could you please base your patch on top of mine?
Thank you,
André
--
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/
Powered by blists - more mailing lists