| 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
| ||
|
Date: Sun, 29 Mar 2015 12:27:44 +0800
From: Boqun Feng <boqun.feng@...il.com>
To: linux-fsdevel@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
Paul Moore <pmoore@...hat.com>
Subject: Re: [PATCH v2] vfs: avoid recopying file names in getname_flags
Ping.
This patch has been tested by 0day test bot.
Thanks,
Boqun Feng
On Thu, Mar 26, 2015 at 02:45:52AM +0800, Boqun Feng wrote:
> In the current implementation of getname_flags, a file name 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 file name are already copied into kernel address space, the only
> reason why the recopy is needed is that "kname", which points to the
> string of the file name, needs to be relocated.
>
> And the recopy can be avoided if we change the memory layout of the
> `names_cachep` allocation. By putting `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.
>
> Of course, other functions aware of the layout of the `names_cachep`
> allocation, i.e., getname_kernel and putname also need to be modified to
> adapt to the new layout.
>
> Since we change the layout of `names_cachep` allocation, in order to
> figure out whether kname and the struct are separate, we now need to
> check whether the file name string starts at the address
> EMBEDDED_NAME_MAX bytes before the address of the struct. As a result,
> `iname`, which points the end of `struct filename`, is not needed
> anymore.
>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> ---
> v1 --> v2:
> Rebase the patch onto the for-next branch of Al's vfs repo.
>
>
> fs/namei.c | 45 ++++++++++++++++++++++++++++-----------------
> include/linux/fs.h | 1 -
> 2 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 7a11ec1..6d04029 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -119,7 +119,7 @@
> * PATH_MAX includes the nul terminator --RR.
> */
>
> -#define EMBEDDED_NAME_MAX (PATH_MAX - offsetof(struct filename, iname))
> +#define EMBEDDED_NAME_MAX (PATH_MAX - sizeof(struct filename))
>
> struct filename *
> getname_flags(const char __user *filename, int flags, int *empty)
> @@ -132,44 +132,53 @@ 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);
>
> /*
> * First, try to embed the struct filename inside the names_cache
> * allocation
> */
> - kname = (char *)result->iname;
> + result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
> result->name = kname;
>
> len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX);
> if (unlikely(len < 0)) {
> - __putname(result);
> + __putname(kname);
> return ERR_PTR(len);
> }
>
> /*
> * 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 (unlikely(len == EMBEDDED_NAME_MAX)) {
> - kname = (char *)result;
> -
> result = kzalloc(sizeof(*result), GFP_KERNEL);
> if (unlikely(!result)) {
> __putname(kname);
> return ERR_PTR(-ENOMEM);
> }
> result->name = kname;
> - len = strncpy_from_user(kname, filename, PATH_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,
> + PATH_MAX - EMBEDDED_NAME_MAX);
> +
> if (unlikely(len < 0)) {
> __putname(kname);
> kfree(result);
> return ERR_PTR(len);
> }
> +
> + len += EMBEDDED_NAME_MAX;
> if (unlikely(len == PATH_MAX)) {
> __putname(kname);
> kfree(result);
> @@ -204,26 +213,28 @@ 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->iname;
> + result = (struct filename *)(kname + EMBEDDED_NAME_MAX);
> + result->name = kname;
> } 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;
> result = tmp;
> } else {
> - __putname(result);
> + __putname(kname);
> return ERR_PTR(-ENAMETOOLONG);
> }
> memcpy((char *)result->name, filename, len);
> @@ -242,11 +253,11 @@ void putname(struct filename *name)
> if (--name->refcnt > 0)
> return;
>
> - if (name->name != name->iname) {
> + if (name->name != ((char *)name - EMBEDDED_NAME_MAX)) {
> __putname(name->name);
> kfree(name);
> } else
> - __putname(name);
> + __putname(name->name);
> }
>
> static int check_acl(struct inode *inode, int mask)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index dfbd88a..dd67284 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2166,7 +2166,6 @@ struct filename {
> const __user char *uptr; /* original userland pointer */
> struct audit_names *aname;
> int refcnt;
> - const char iname[];
> };
>
> extern long vfs_truncate(struct path *, loff_t);
> --
> 2.3.3
>
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists