| 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: <CAL0jBu-9wF665YX22em6qiittScfXM_eQVteMGqk6kZs234geA@mail.gmail.com>
Date: Mon, 9 Mar 2015 16:24:32 +0800
From: Boqun Feng <boqun.feng@...il.com>
To: linux-fsdevel@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Boqun Feng <boqun.feng@...il.com>,
Al Viro <viro@...iv.linux.org.uk>,
Paul Moore <pmoore@...hat.com>
Subject: Re: [PATCH] vfs: avoid recopying filename in getname_flags
Ping.
Any opinion?
Thanks,
Boqun Feng
On Wed, Feb 25, 2015 at 8:31 PM, Boqun Feng <boqun.feng@...il.com> wrote:
> In the current implementation of getname_flags, filename in the
> user-space will be recopied if it takes more space that
> EMBEDDED_NAME_MAX, however, at this moment, EMBEDDED_NAME_MAX bytes of
> the filename are already copied into kernel space, the only reason why
> the recopy is needed is that "kname" needs to be relocated.
>
> And the recopy can be avoided if we change the memory layout of the
> "names_cache" allocation. By putting the struct "filename" at the tail
> of the allocation instead of the head, relocation of kname is avoided.
>
> Once putting the struct at the tail, each byte in the user space will be
> copied only one time, so the recopy is avoided and code is more clear.
>
> Of course, other functions aware of the layout of the names_cache
> allocation, i.e., getname_kernel and putname also need to be modified to
> adapt to the new layout.
>
> This patch is based on v4.0-rc1.
>
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: Paul Moore <pmoore@...hat.com>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> ---
> fs/namei.c | 50 +++++++++++++++++++++++++++++++-------------------
> 1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index c83145a..3be372b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -133,21 +133,20 @@ getname_flags(const char __user *filename, int flags, int *empty)
> if (result)
> return result;
>
> - result = __getname();
> - if (unlikely(!result))
> + kname = __getname();
> + if (unlikely(!kname))
> return ERR_PTR(-ENOMEM);
> - result->refcnt = 1;
>
> /*
> * First, try to embed the struct filename inside the names_cache
> * allocation
> */
> - kname = (char *)result + sizeof(*result);
> + result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
> result->name = kname;
> result->separate = false;
> + result->refcnt = 1;
> max = EMBEDDED_NAME_MAX;
>
> -recopy:
> len = strncpy_from_user(kname, filename, max);
> if (unlikely(len < 0)) {
> err = ERR_PTR(len);
> @@ -157,23 +156,34 @@ recopy:
> /*
> * Uh-oh. We have a name that's approaching PATH_MAX. Allocate a
> * separate struct filename so we can dedicate the entire
> - * names_cache allocation for the pathname, and re-do the copy from
> + * names_cache allocation for the pathname, and continue the copy from
> * userland.
> */
> - if (len == EMBEDDED_NAME_MAX && max == EMBEDDED_NAME_MAX) {
> - kname = (char *)result;
> -
> + if (len == EMBEDDED_NAME_MAX) {
> result = kzalloc(sizeof(*result), GFP_KERNEL);
> if (!result) {
> err = ERR_PTR(-ENOMEM);
> - result = (struct filename *)kname;
> + result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
> goto error;
> }
> result->name = kname;
> result->separate = true;
> result->refcnt = 1;
> - max = PATH_MAX;
> - goto recopy;
> + max = PATH_MAX - EMBEDDED_NAME_MAX;
> + /* we can't simply add the number of rest chars we copy to len,
> + * since strncpy_from_user may return negative to indicate
> + * something is wrong, so we do the addition later, after
> + * strncpy_from_user succeeds, and we know we already copy
> + * EMBEDDED_NAME_MAX chars.
> + */
> + len = strncpy_from_user(kname + EMBEDDED_NAME_MAX,
> + filename + EMBEDDED_NAME_MAX, max);
> +
> + if (unlikely(len < 0)) {
> + err = ERR_PTR(len);
> + goto error;
> + }
> + len += EMBEDDED_NAME_MAX;
> }
>
> /* The empty path is special. */
> @@ -209,28 +219,30 @@ struct filename *
> getname_kernel(const char * filename)
> {
> struct filename *result;
> + char *kname;
> int len = strlen(filename) + 1;
>
> - result = __getname();
> - if (unlikely(!result))
> + kname = __getname();
> + if (unlikely(!kname))
> return ERR_PTR(-ENOMEM);
>
> if (len <= EMBEDDED_NAME_MAX) {
> - result->name = (char *)(result) + sizeof(*result);
> + result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
> + result->name = kname;
> result->separate = false;
> } else if (len <= PATH_MAX) {
> struct filename *tmp;
>
> tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> if (unlikely(!tmp)) {
> - __putname(result);
> + __putname(kname);
> return ERR_PTR(-ENOMEM);
> }
> - tmp->name = (char *)result;
> + tmp->name = kname;
> tmp->separate = true;
> result = tmp;
> } else {
> - __putname(result);
> + __putname(kname);
> return ERR_PTR(-ENAMETOOLONG);
> }
> memcpy((char *)result->name, filename, len);
> @@ -253,7 +265,7 @@ void putname(struct filename *name)
> __putname(name->name);
> kfree(name);
> } else
> - __putname(name);
> + __putname(name->name);
> }
>
> static int check_acl(struct inode *inode, int mask)
> --
> 2.3.0
>
--
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