| 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: Tue, 7 Apr 2015 16:38:26 +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 again...
Thanks,
Boqun Feng
On Sun, Mar 29, 2015 at 12:27:44PM +0800, Boqun Feng wrote:
> 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