From 0766ec82e5fb26fc5dc6d592bc61865608bdc651 Mon Sep 17 00:00:00 2001 From: Stephen Brennan Date: Wed, 1 Sep 2021 10:51:41 -0700 Subject: [PATCH 1/5] namei: Fix use after free in kern_path_locked In 0ee50b47532a ("namei: change filename_parentat() calling conventions"), filename_parentat() was made to always call putname() on the filename before returning, and kern_path_locked() was migrated to this calling convention. However, kern_path_locked() uses the "last" parameter to lookup and potentially create a new dentry. The last parameter contains the last component of the path and points within the filename, which was recently freed at the end of filename_parentat(). Thus, when kern_path_locked() calls __lookup_hash(), it is using the filename after it has already been freed. In other words, these calling conventions had been wrong for the only remaining caller of filename_parentat(). Everything else is using __filename_parentat(), which does not drop the reference; so should kern_path_locked(). Switch kern_path_locked() to use of __filename_parentat() and move getting/dropping struct filename into wrapper. Remove filename_parentat(), now that we have no remaining callers. Fixes: 0ee50b47532a ("namei: change filename_parentat() calling conventions") Link: https://lore.kernel.org/linux-fsdevel/YS9D4AlEsaCxLFV0@infradead.org/ Link: https://lore.kernel.org/linux-fsdevel/YS+csMTV2tTXKg3s@zeniv-ca.linux.org.uk/ Cc: Christoph Hellwig Cc: Al Viro Reported-by: syzbot+fb0d60a179096e8c2731@syzkaller.appspotmail.com Signed-off-by: Stephen Brennan Co-authored-by: Dmitry Kadashev Signed-off-by: Al Viro --- fs/namei.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index 95a881e0552b1d..33a2a850409961 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2514,9 +2514,10 @@ static int path_parentat(struct nameidata *nd, unsigned flags, return err; } +/* Note: this does not consume "name" */ static int __filename_parentat(int dfd, struct filename *name, - unsigned int flags, struct path *parent, - struct qstr *last, int *type) + unsigned int flags, struct path *parent, + struct qstr *last, int *type) { int retval; struct nameidata nd; @@ -2538,25 +2539,14 @@ static int __filename_parentat(int dfd, struct filename *name, return retval; } -static int filename_parentat(int dfd, struct filename *name, - unsigned int flags, struct path *parent, - struct qstr *last, int *type) -{ - int retval = __filename_parentat(dfd, name, flags, parent, last, type); - - putname(name); - return retval; -} - /* does lookup, returns the object with parent locked */ -struct dentry *kern_path_locked(const char *name, struct path *path) +static struct dentry *__kern_path_locked(struct filename *name, struct path *path) { struct dentry *d; struct qstr last; int type, error; - error = filename_parentat(AT_FDCWD, getname_kernel(name), 0, path, - &last, &type); + error = __filename_parentat(AT_FDCWD, name, 0, path, &last, &type); if (error) return ERR_PTR(error); if (unlikely(type != LAST_NORM)) { @@ -2572,6 +2562,15 @@ struct dentry *kern_path_locked(const char *name, struct path *path) return d; } +struct dentry *kern_path_locked(const char *name, struct path *path) +{ + struct filename *filename = getname_kernel(name); + struct dentry *res = __kern_path_locked(filename, path); + + putname(filename); + return res; +} + int kern_path(const char *name, unsigned int flags, struct path *path) { return filename_lookup(AT_FDCWD, getname_kernel(name), From c5f563f9e9e66c0ad0b23abe25165c124579b70e Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 7 Sep 2021 15:57:42 -0400 Subject: [PATCH 2/5] rename __filename_parentat() to filename_parentat() ... in separate commit, to avoid noise in previous one Signed-off-by: Al Viro --- fs/namei.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index 33a2a850409961..56b5860ebe3252 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2515,7 +2515,7 @@ static int path_parentat(struct nameidata *nd, unsigned flags, } /* Note: this does not consume "name" */ -static int __filename_parentat(int dfd, struct filename *name, +static int filename_parentat(int dfd, struct filename *name, unsigned int flags, struct path *parent, struct qstr *last, int *type) { @@ -2546,7 +2546,7 @@ static struct dentry *__kern_path_locked(struct filename *name, struct path *pat struct qstr last; int type, error; - error = __filename_parentat(AT_FDCWD, name, 0, path, &last, &type); + error = filename_parentat(AT_FDCWD, name, 0, path, &last, &type); if (error) return ERR_PTR(error); if (unlikely(type != LAST_NORM)) { @@ -3633,7 +3633,7 @@ static struct dentry *__filename_create(int dfd, struct filename *name, */ lookup_flags &= LOOKUP_REVAL; - error = __filename_parentat(dfd, name, lookup_flags, path, &last, &type); + error = filename_parentat(dfd, name, lookup_flags, path, &last, &type); if (error) return ERR_PTR(error); @@ -3995,7 +3995,7 @@ int do_rmdir(int dfd, struct filename *name) int type; unsigned int lookup_flags = 0; retry: - error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type); + error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type); if (error) goto exit1; @@ -4136,7 +4136,7 @@ int do_unlinkat(int dfd, struct filename *name) struct inode *delegated_inode = NULL; unsigned int lookup_flags = 0; retry: - error = __filename_parentat(dfd, name, lookup_flags, &path, &last, &type); + error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type); if (error) goto exit1; @@ -4688,13 +4688,13 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd, target_flags = 0; retry: - error = __filename_parentat(olddfd, from, lookup_flags, &old_path, - &old_last, &old_type); + error = filename_parentat(olddfd, from, lookup_flags, &old_path, + &old_last, &old_type); if (error) goto put_names; - error = __filename_parentat(newdfd, to, lookup_flags, &new_path, &new_last, - &new_type); + error = filename_parentat(newdfd, to, lookup_flags, &new_path, &new_last, + &new_type); if (error) goto exit1; From 794ebcea865bff47231de89269e9d542121ab7be Mon Sep 17 00:00:00 2001 From: Stephen Brennan Date: Wed, 1 Sep 2021 10:51:42 -0700 Subject: [PATCH 3/5] namei: Standardize callers of filename_lookup() filename_lookup() has two variants, one which drops the caller's reference to filename (filename_lookup), and one which does not (__filename_lookup). This can be confusing as it's unusual to drop a caller's reference. Remove filename_lookup, rename __filename_lookup to filename_lookup, and convert all callers. The cost is a few slightly longer functions, but the clarity is greater. [AV: consuming a reference is not at all unusual, actually; look at e.g. do_mkdirat(), for example. It's more that we want non-consuming variant for close relative of that function...] Link: https://lore.kernel.org/linux-fsdevel/YS+dstZ3xfcLxhoB@zeniv-ca.linux.org.uk/ Cc: Christoph Hellwig Cc: Al Viro Signed-off-by: Stephen Brennan Signed-off-by: Al Viro --- fs/fs_parser.c | 1 - fs/namei.c | 37 ++++++++++++++++++++----------------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/fs/fs_parser.c b/fs/fs_parser.c index 980d44fd3a3639..3df07c0e32b34a 100644 --- a/fs/fs_parser.c +++ b/fs/fs_parser.c @@ -165,7 +165,6 @@ int fs_lookup_param(struct fs_context *fc, return invalf(fc, "%s: not usable as path", param->key); } - f->refcnt++; /* filename_lookup() drops our ref. */ ret = filename_lookup(param->dirfd, f, flags, _path, NULL); if (ret < 0) { errorf(fc, "%s: Lookup failure for '%s'", param->key, f->name); diff --git a/fs/namei.c b/fs/namei.c index 56b5860ebe3252..7181b5841388d1 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2467,7 +2467,7 @@ static int path_lookupat(struct nameidata *nd, unsigned flags, struct path *path return err; } -static int __filename_lookup(int dfd, struct filename *name, unsigned flags, +int filename_lookup(int dfd, struct filename *name, unsigned flags, struct path *path, struct path *root) { int retval; @@ -2488,15 +2488,6 @@ static int __filename_lookup(int dfd, struct filename *name, unsigned flags, return retval; } -int filename_lookup(int dfd, struct filename *name, unsigned flags, - struct path *path, struct path *root) -{ - int retval = __filename_lookup(dfd, name, flags, path, root); - - putname(name); - return retval; -} - /* Returns 0 and nd will be valid on success; Retuns error, otherwise. */ static int path_parentat(struct nameidata *nd, unsigned flags, struct path *parent) @@ -2573,8 +2564,12 @@ struct dentry *kern_path_locked(const char *name, struct path *path) int kern_path(const char *name, unsigned int flags, struct path *path) { - return filename_lookup(AT_FDCWD, getname_kernel(name), - flags, path, NULL); + struct filename *filename = getname_kernel(name); + int ret = filename_lookup(AT_FDCWD, filename, flags, path, NULL); + + putname(filename); + return ret; + } EXPORT_SYMBOL(kern_path); @@ -2590,10 +2585,15 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt, const char *name, unsigned int flags, struct path *path) { + struct filename *filename; struct path root = {.mnt = mnt, .dentry = dentry}; + int ret; + + filename = getname_kernel(name); /* the first argument of filename_lookup() is ignored with root */ - return filename_lookup(AT_FDCWD, getname_kernel(name), - flags , path, &root); + ret = filename_lookup(AT_FDCWD, filename, flags, path, &root); + putname(filename); + return ret; } EXPORT_SYMBOL(vfs_path_lookup); @@ -2797,8 +2797,11 @@ int path_pts(struct path *path) int user_path_at_empty(int dfd, const char __user *name, unsigned flags, struct path *path, int *empty) { - return filename_lookup(dfd, getname_flags(name, flags, empty), - flags, path, NULL); + struct filename *filename = getname_flags(name, flags, empty); + int ret = filename_lookup(dfd, filename, flags, path, NULL); + + putname(filename); + return ret; } EXPORT_SYMBOL(user_path_at_empty); @@ -4425,7 +4428,7 @@ int do_linkat(int olddfd, struct filename *old, int newdfd, if (flags & AT_SYMLINK_FOLLOW) how |= LOOKUP_FOLLOW; retry: - error = __filename_lookup(olddfd, old, how, &old_path, NULL); + error = filename_lookup(olddfd, old, how, &old_path, NULL); if (error) goto out_putnames; From b4a4f213a39d5e55baf38c96042acaeaf927ec74 Mon Sep 17 00:00:00 2001 From: Stephen Brennan Date: Wed, 1 Sep 2021 10:51:43 -0700 Subject: [PATCH 4/5] namei: Standardize callers of filename_create() filename_create() has two variants, one which drops the caller's reference to filename (filename_create) and one which does not (__filename_create). This can be confusing as it's unusual to drop a caller's reference. Remove filename_create, rename __filename_create to filename_create, and convert all callers. Link: https://lore.kernel.org/linux-fsdevel/f6238254-35bd-7e97-5b27-21050c745874@oracle.com/ Cc: Christoph Hellwig Cc: Al Viro Signed-off-by: Stephen Brennan Signed-off-by: Al Viro --- fs/namei.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index 7181b5841388d1..bbb5c9bf5c61e2 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3620,8 +3620,8 @@ struct file *do_file_open_root(const struct path *root, return file; } -static struct dentry *__filename_create(int dfd, struct filename *name, - struct path *path, unsigned int lookup_flags) +static struct dentry *filename_create(int dfd, struct filename *name, + struct path *path, unsigned int lookup_flags) { struct dentry *dentry = ERR_PTR(-EEXIST); struct qstr last; @@ -3689,21 +3689,15 @@ static struct dentry *__filename_create(int dfd, struct filename *name, return dentry; } -static inline struct dentry *filename_create(int dfd, struct filename *name, +struct dentry *kern_path_create(int dfd, const char *pathname, struct path *path, unsigned int lookup_flags) { - struct dentry *res = __filename_create(dfd, name, path, lookup_flags); + struct filename *filename = getname_kernel(pathname); + struct dentry *res = filename_create(dfd, filename, path, lookup_flags); - putname(name); + putname(filename); return res; } - -struct dentry *kern_path_create(int dfd, const char *pathname, - struct path *path, unsigned int lookup_flags) -{ - return filename_create(dfd, getname_kernel(pathname), - path, lookup_flags); -} EXPORT_SYMBOL(kern_path_create); void done_path_create(struct path *path, struct dentry *dentry) @@ -3718,7 +3712,11 @@ EXPORT_SYMBOL(done_path_create); inline struct dentry *user_path_create(int dfd, const char __user *pathname, struct path *path, unsigned int lookup_flags) { - return filename_create(dfd, getname(pathname), path, lookup_flags); + struct filename *filename = getname(pathname); + struct dentry *res = filename_create(dfd, filename, path, lookup_flags); + + putname(filename); + return res; } EXPORT_SYMBOL(user_path_create); @@ -3799,7 +3797,7 @@ static int do_mknodat(int dfd, struct filename *name, umode_t mode, if (error) goto out1; retry: - dentry = __filename_create(dfd, name, &path, lookup_flags); + dentry = filename_create(dfd, name, &path, lookup_flags); error = PTR_ERR(dentry); if (IS_ERR(dentry)) goto out1; @@ -3899,7 +3897,7 @@ int do_mkdirat(int dfd, struct filename *name, umode_t mode) unsigned int lookup_flags = LOOKUP_DIRECTORY; retry: - dentry = __filename_create(dfd, name, &path, lookup_flags); + dentry = filename_create(dfd, name, &path, lookup_flags); error = PTR_ERR(dentry); if (IS_ERR(dentry)) goto out_putname; @@ -4268,7 +4266,7 @@ int do_symlinkat(struct filename *from, int newdfd, struct filename *to) goto out_putnames; } retry: - dentry = __filename_create(newdfd, to, &path, lookup_flags); + dentry = filename_create(newdfd, to, &path, lookup_flags); error = PTR_ERR(dentry); if (IS_ERR(dentry)) goto out_putnames; @@ -4432,7 +4430,7 @@ int do_linkat(int olddfd, struct filename *old, int newdfd, if (error) goto out_putnames; - new_dentry = __filename_create(newdfd, new, &new_path, + new_dentry = filename_create(newdfd, new, &new_path, (how & LOOKUP_REVAL)); error = PTR_ERR(new_dentry); if (IS_ERR(new_dentry)) From ea47ab111669b187808b3080602788dec26cb9bc Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 7 Sep 2021 16:14:05 -0400 Subject: [PATCH 5/5] putname(): IS_ERR_OR_NULL() is wrong here Mixing NULL and ERR_PTR() just in case is a Bad Idea(tm). For struct filename the former is wrong - failures are reported as ERR_PTR(...), not as NULL. Signed-off-by: Al Viro --- fs/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/namei.c b/fs/namei.c index bbb5c9bf5c61e2..1946d966779088 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -255,7 +255,7 @@ getname_kernel(const char * filename) void putname(struct filename *name) { - if (IS_ERR_OR_NULL(name)) + if (IS_ERR(name)) return; BUG_ON(name->refcnt <= 0);