Drawing from the comments on the last two patches from me and Dmitry,
the concensus is that __filename_parentat() is inherently buggy, and
should be removed. But there's some nice consistency to the way that
the other functions (filename_create, filename_lookup) are named which
would get broken.
I looked at the callers of filename_create and filename_lookup. All are
small functions which are trivial to modify to include a putname(). It
seems to me that adding a few more lines to these functions is a good
traedoff for better clarity on lifetimes (as it's uncommon for functions
to drop references to their parameters) and better consistency.
This small series combines the UAF fix from me, and the removal of
__filename_parentat() from Dmitry as patch 1. Then I standardize
filename_create() and filename_lookup() and their callers.
v2: Fix double getname in user_path_create()
Stephen Brennan (3):
namei: Fix use after free in kern_path_locked
namei: Standardize callers of filename_lookup()
namei: Standardize callers of filename_create()
fs/fs_parser.c | 1 -
fs/namei.c | 126 ++++++++++++++++++++++++++-----------------------
2 files changed, 66 insertions(+), 61 deletions(-)
--
2.30.2
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 this case, filename_parentat() is fundamentally broken due to this
use after free. So, remove it, and rename __filename_parentat to
filename_parentat, migrating all callers. Adjust kern_path_locked to put
the filename once all users are done with it.
Fixes: 0ee50b47532a ("namei: change filename_parentat() calling conventions")
Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Reported-by: [email protected]
Signed-off-by: Stephen Brennan <[email protected]>
Co-authored-by: Dmitry Kadashev <[email protected]>
---
fs/namei.c | 47 ++++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 25 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index d049d3972695..f2af301cc79f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2514,9 +2514,10 @@ static int path_parentat(struct nameidata *nd, unsigned flags,
return err;
}
-static int __filename_parentat(int dfd, struct filename *name,
- unsigned int flags, struct path *parent,
- struct qstr *last, int *type)
+/* 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)
{
int retval;
struct nameidata nd;
@@ -2538,30 +2539,24 @@ 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)
{
+ struct filename *filename;
struct dentry *d;
struct qstr last;
int type, error;
- error = filename_parentat(AT_FDCWD, getname_kernel(name), 0, path,
- &last, &type);
- if (error)
- return ERR_PTR(error);
+ filename = getname_kernel(name);
+ error = filename_parentat(AT_FDCWD, filename, 0, path, &last, &type);
+ if (error) {
+ d = ERR_PTR(error);
+ goto out;
+ }
if (unlikely(type != LAST_NORM)) {
path_put(path);
- return ERR_PTR(-EINVAL);
+ d = ERR_PTR(-EINVAL);
+ goto out;
}
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
d = __lookup_hash(&last, path->dentry, 0);
@@ -2569,6 +2564,8 @@ struct dentry *kern_path_locked(const char *name, struct path *path)
inode_unlock(path->dentry->d_inode);
path_put(path);
}
+out:
+ putname(filename);
return d;
}
@@ -3634,7 +3631,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);
@@ -3996,7 +3993,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;
@@ -4135,7 +4132,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;
@@ -4683,13 +4680,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;
--
2.30.2
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/[email protected]/
Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Signed-off-by: Stephen Brennan <[email protected]>
---
fs/namei.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 76871b7f127a..0d3ce04c55ad 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3622,8 +3622,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;
@@ -3691,20 +3691,16 @@ static struct dentry *__filename_create(int dfd, struct filename *name,
return dentry;
}
-static inline struct dentry *filename_create(int dfd, struct filename *name,
- struct path *path, unsigned int lookup_flags)
-{
- struct dentry *res = __filename_create(dfd, name, path, lookup_flags);
-
- putname(name);
- 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);
+ struct filename *filename;
+ struct dentry *dentry;
+
+ filename = getname_kernel(pathname);
+ dentry = filename_create(dfd, filename, path, lookup_flags);
+ putname(filename);
+ return dentry;
}
EXPORT_SYMBOL(kern_path_create);
@@ -3720,7 +3716,13 @@ 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;
+ struct dentry *dentry;
+
+ filename = getname(pathname);
+ dentry = filename_create(dfd, pathname, path, lookup_flags);
+ putname(filename);
+ return dentry;
}
EXPORT_SYMBOL(user_path_create);
@@ -3801,7 +3803,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;
@@ -3901,7 +3903,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 +4270,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 +4434,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))
--
2.30.2
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.
Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
Cc: Christoph Hellwig <[email protected]>
Cc: Al Viro <[email protected]>
Signed-off-by: Stephen Brennan <[email protected]>
---
fs/fs_parser.c | 1 -
fs/namei.c | 41 ++++++++++++++++++++++++-----------------
2 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/fs/fs_parser.c b/fs/fs_parser.c
index 980d44fd3a36..3df07c0e32b3 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 f2af301cc79f..76871b7f127a 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)
@@ -2571,8 +2562,14 @@ 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;
+ int ret;
+
+ filename = getname_kernel(name);
+ ret = filename_lookup(AT_FDCWD, filename, flags, path, NULL);
+ putname(filename);
+ return ret;
+
}
EXPORT_SYMBOL(kern_path);
@@ -2588,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);
@@ -2795,8 +2797,13 @@ 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;
+ int ret;
+
+ filename = getname_flags(name, flags, empty);
+ ret = filename_lookup(dfd, filename, flags, path, NULL);
+ putname(filename);
+ return ret;
}
EXPORT_SYMBOL(user_path_at_empty);
@@ -4421,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;
--
2.30.2
Stephen Brennan <[email protected]> writes:
> Drawing from the comments on the last two patches from me and Dmitry,
> the concensus is that __filename_parentat() is inherently buggy, and
> should be removed. But there's some nice consistency to the way that
> the other functions (filename_create, filename_lookup) are named which
> would get broken.
>
> I looked at the callers of filename_create and filename_lookup. All are
> small functions which are trivial to modify to include a putname(). It
> seems to me that adding a few more lines to these functions is a good
> traedoff for better clarity on lifetimes (as it's uncommon for functions
> to drop references to their parameters) and better consistency.
>
> This small series combines the UAF fix from me, and the removal of
> __filename_parentat() from Dmitry as patch 1. Then I standardize
> filename_create() and filename_lookup() and their callers.
>
> v2: Fix double getname in user_path_create()
>
> Stephen Brennan (3):
> namei: Fix use after free in kern_path_locked
> namei: Standardize callers of filename_lookup()
> namei: Standardize callers of filename_create()
Please ignore this! I missed some feedback from Al and will send a v3.
>
> fs/fs_parser.c | 1 -
> fs/namei.c | 126 ++++++++++++++++++++++++++-----------------------
> 2 files changed, 66 insertions(+), 61 deletions(-)
>
> --
> 2.30.2