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: <20180408031646.GB32632@bombadil.infradead.org> Date: Sat, 7 Apr 2018 20:16:46 -0700 From: Matthew Wilcox <willy@...radead.org> To: Dmitry Vyukov <dvyukov@...gle.com> Cc: linux-fsdevel <linux-fsdevel@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, Al Viro <viro@...iv.linux.org.uk> Subject: Use struct page for filename On Fri, Apr 06, 2018 at 03:33:36PM +0200, Dmitry Vyukov wrote: > On Fri, Apr 6, 2018 at 3:24 PM, syzbot > <syzbot+75397ee3df5c70164154@...kaller.appspotmail.com> wrote: > > cache_from_obj: Wrong slab cache. names_cache but object is from kmalloc-96 > > Al, do you see how this can happen? I don't see how it happened, but when looking at this bug, I thought "This is very complicated, I think there's a simpler way to handle this". Here's a proposal. It won't apply to any existing tree (depends on a couple of local commits), but I think you'll get the general flavour of it. It's mostly compile-tested (build still running, but fs/ and kernel/ compiled without issue). fs/dcache.c | 7 ---- fs/namei.c | 102 ++++++++++++----------------------------------- include/linux/fs.h | 26 ++++++------ include/linux/mm_types.h | 12 +++++- kernel/auditsc.c | 8 ++-- 5 files changed, 51 insertions(+), 104 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index 593079176123..749b82b8fa1c 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -3172,10 +3172,6 @@ static void __init dcache_init(void) d_hash_shift = 32 - d_hash_shift; } -/* SLAB cache for __getname() consumers */ -struct kmem_cache *names_cachep __read_mostly; -EXPORT_SYMBOL(names_cachep); - void __init vfs_caches_init_early(void) { int i; @@ -3189,9 +3185,6 @@ void __init vfs_caches_init_early(void) void __init vfs_caches_init(void) { - names_cachep = kmem_cache_create_usercopy("names_cache", PATH_MAX, 0, - SLAB_HWCACHE_ALIGN|SLAB_PANIC, 0, PATH_MAX, NULL); - dcache_init(); inode_init(); files_init(); diff --git a/fs/namei.c b/fs/namei.c index a09419379f5d..16fb4779d29f 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -122,71 +122,46 @@ * PATH_MAX includes the nul terminator --RR. */ -#define EMBEDDED_NAME_MAX (PATH_MAX - offsetof(struct filename, iname)) +struct filename *alloc_filename(void) +{ + struct page *page = alloc_page(GFP_KERNEL); + + page->filename.name = page_to_virt(page); + return &page->filename; +} + +void putname(struct filename *name) +{ + __free_page(container_of(name, struct page, filename)); +} + +void filename_get(struct filename *name) +{ + page_ref_inc(container_of(name, struct page, filename)); +} struct filename * getname_flags(const char __user *filename, int flags, int *empty) { struct filename *result; - char *kname; int len; result = audit_reusename(filename); if (result) return result; - result = __getname(); + result = alloc_filename(); if (unlikely(!result)) return ERR_PTR(-ENOMEM); - /* - * First, try to embed the struct filename inside the names_cache - * allocation - */ - kname = (char *)result->iname; - result->name = kname; - - len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX); + len = strncpy_from_user((char *)result->name, filename, PATH_MAX); + if (unlikely(len == PATH_MAX)) + len = -ENAMETOOLONG; if (unlikely(len < 0)) { - __putname(result); + putname(result); 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 - * userland. - */ - if (unlikely(len == EMBEDDED_NAME_MAX)) { - const size_t size = offsetof(struct filename, iname[1]); - kname = (char *)result; - - /* - * size is chosen that way we to guarantee that - * result->iname[0] is within the same object and that - * kname can't be equal to result->iname, no matter what. - */ - result = kzalloc(size, GFP_KERNEL); - if (unlikely(!result)) { - __putname(kname); - return ERR_PTR(-ENOMEM); - } - result->name = kname; - len = strncpy_from_user(kname, filename, PATH_MAX); - if (unlikely(len < 0)) { - __putname(kname); - kfree(result); - return ERR_PTR(len); - } - if (unlikely(len == PATH_MAX)) { - __putname(kname); - kfree(result); - return ERR_PTR(-ENAMETOOLONG); - } - } - - result->refcnt = 1; /* The empty path is special. */ if (unlikely(!len)) { if (empty) @@ -215,49 +190,22 @@ getname_kernel(const char * filename) struct filename *result; int len = strlen(filename) + 1; - result = __getname(); + result = alloc_filename(); if (unlikely(!result)) return ERR_PTR(-ENOMEM); - if (len <= EMBEDDED_NAME_MAX) { - result->name = (char *)result->iname; - } else if (len <= PATH_MAX) { - struct filename *tmp; - - tmp = kmalloc(sizeof(*tmp), GFP_KERNEL); - if (unlikely(!tmp)) { - __putname(result); - return ERR_PTR(-ENOMEM); - } - tmp->name = (char *)result; - result = tmp; - } else { - __putname(result); + if (len > PATH_MAX) { + putname(result); return ERR_PTR(-ENAMETOOLONG); } memcpy((char *)result->name, filename, len); result->uptr = NULL; result->aname = NULL; - result->refcnt = 1; audit_getname(result); return result; } -void putname(struct filename *name) -{ - BUG_ON(name->refcnt <= 0); - - if (--name->refcnt > 0) - return; - - if (name->name != name->iname) { - __putname(name->name); - kfree(name); - } else - __putname(name); -} - static int check_acl(struct inode *inode, int mask) { #ifdef CONFIG_FS_POSIX_ACL diff --git a/include/linux/fs.h b/include/linux/fs.h index ab44a19f2ddd..5c93ebc519fe 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2376,15 +2376,6 @@ static inline int break_layout(struct inode *inode, bool wait) #endif /* CONFIG_FILE_LOCKING */ /* fs/open.c */ -struct audit_names; -struct filename { - const char *name; /* pointer to actual string */ - const __user char *uptr; /* original userland pointer */ - struct audit_names *aname; - int refcnt; - const char iname[]; -}; - extern long vfs_truncate(const struct path *, loff_t); extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs, struct file *filp); @@ -2399,6 +2390,18 @@ extern struct file *file_open_root(struct dentry *, struct vfsmount *, extern struct file * dentry_open(const struct path *, int, const struct cred *); extern int filp_close(struct file *, fl_owner_t id); +/* fs/namei.c */ +static inline void *__getname(void) +{ + return (void *)__get_free_page(GFP_KERNEL); +} + +static inline void __putname(const void *name) +{ + free_page((unsigned long)name); +} + +void filename_get(struct filename *); extern struct filename *getname_flags(const char __user *, int, int *); extern struct filename *getname(const char __user *); extern struct filename *getname_kernel(const char *); @@ -2421,11 +2424,6 @@ extern int ioctl_preallocate(struct file *filp, void __user *argp); extern void __init vfs_caches_init_early(void); extern void __init vfs_caches_init(void); -extern struct kmem_cache *names_cachep; - -#define __getname() kmem_cache_alloc(names_cachep, GFP_KERNEL) -#define __putname(name) kmem_cache_free(names_cachep, (void *)(name)) - #ifdef CONFIG_BLOCK extern int register_blkdev(unsigned int, const char *); extern void unregister_blkdev(unsigned int, const char *); diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h index 97ceec1c6e21..a6ca28ef4277 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -25,6 +25,13 @@ struct address_space; struct mem_cgroup; struct hmm; +struct audit_names; + +struct filename { + const char *name; /* pointer to actual string */ + const __user char *uptr; /* original userland pointer */ + struct audit_names *aname; +}; /* * Each physical page in the system has a struct page associated with @@ -188,6 +195,7 @@ struct page { spinlock_t ptl; #endif }; + struct filename filename; }; #ifdef CONFIG_MEMCG diff --git a/kernel/auditsc.c b/kernel/auditsc.c index e80459f7e132..e539550f5983 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -1722,7 +1722,7 @@ __audit_reusename(const __user char *uptr) if (!n->name) continue; if (n->name->uptr == uptr) { - n->name->refcnt++; + filename_get(n->name); return n->name; } } @@ -1751,7 +1751,7 @@ void __audit_getname(struct filename *name) n->name = name; n->name_len = AUDIT_NAME_FULL; name->aname = n; - name->refcnt++; + filename_get(name); if (!context->pwd.dentry) get_fs_pwd(current->fs, &context->pwd); @@ -1825,7 +1825,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry, return; if (name) { n->name = name; - name->refcnt++; + filename_get(name); } out: @@ -1954,7 +1954,7 @@ void __audit_inode_child(struct inode *parent, if (found_parent) { found_child->name = found_parent->name; found_child->name_len = AUDIT_NAME_FULL; - found_child->name->refcnt++; + filename_get(found_child->name); } }
Powered by blists - more mailing lists