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.
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
On Wed, Sep 01, 2021 at 10:51:40AM -0700, Stephen Brennan wrote:
> 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.
For kern_path_locked() itself, I'd probably go for
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, name, 0, path,
&last, &type);
if (error)
return ERR_PTR(error);
if (unlikely(type != LAST_NORM)) {
path_put(path);
return ERR_PTR(-EINVAL);
}
inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
d = __lookup_hash(&last, path->dentry, 0);
if (IS_ERR(d)) {
inode_unlock(path->dentry->d_inode);
path_put(path);
}
return d;
}
static 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;
}
instead of that messing with gotos - and split renaming from fix in that
commit. In 3/3 you have a leak; trivial to fix, fortunately.
Another part I really dislike in that area (not your fault, obviously)
is
void putname(struct filename *name)
{
if (IS_ERR_OR_NULL(name))
return;
in mainline right now. Could somebody explain when the hell has NULL
become a possibility here? OK, I buy putname(ERR_PTR(...)) being
a no-op, but IME every sodding time we mixed NULL and ERR_PTR() in
an API we ended up with headache later.
IS_ERR_OR_NULL() is almost always wrong. NULL as argument
for destructor makes sense when constructor can fail with NULL;
not the case here.
How about the variant in vfs.git#misc.namei?
Al Viro <[email protected]> writes:
> On Wed, Sep 01, 2021 at 10:51:40AM -0700, Stephen Brennan wrote:
>> 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.
>
> For kern_path_locked() itself, I'd probably go for
>
> 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, name, 0, path,
> &last, &type);
> if (error)
> return ERR_PTR(error);
> if (unlikely(type != LAST_NORM)) {
> path_put(path);
> return ERR_PTR(-EINVAL);
> }
> inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> d = __lookup_hash(&last, path->dentry, 0);
> if (IS_ERR(d)) {
> inode_unlock(path->dentry->d_inode);
> path_put(path);
> }
> return d;
> }
>
> static 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;
> }
>
> instead of that messing with gotos - and split renaming from fix in that
> commit. In 3/3 you have a leak; trivial to fix, fortunately.
Got it. My v2 crossed paths with your message here, it only fixes the
leak but not the kern_path_locked() change and split. Please ignore it
and I'll adjust patch 1 for v3.
>
> Another part I really dislike in that area (not your fault, obviously)
> is
>
> void putname(struct filename *name)
> {
> if (IS_ERR_OR_NULL(name))
> return;
>
> in mainline right now. Could somebody explain when the hell has NULL
> become a possibility here? OK, I buy putname(ERR_PTR(...)) being
> a no-op, but IME every sodding time we mixed NULL and ERR_PTR() in
> an API we ended up with headache later.
From the links in the blame it seems this was suggested by Linus
here[1]. The core frustration having been with the state of
__filename_create() and friends freeing filenames at different times
depending on whether an error occurred.
[1] https://lore.kernel.org/io-uring/CAHk-=wgCac9hBsYzKMpHk0EbLgQaXR=OUAjHaBtaY+G8A9KhFg@mail.gmail.com/
Thanks,
Stephen
> IS_ERR_OR_NULL() is almost always wrong. NULL as argument
> for destructor makes sense when constructor can fail with NULL;
> not the case here.
>
> How about the variant in vfs.git#misc.namei?
On Tue, Sep 07, 2021 at 02:43:48PM -0700, Stephen Brennan wrote:
> >From the links in the blame it seems this was suggested by Linus
> here[1]. The core frustration having been with the state of
> __filename_create() and friends freeing filenames at different times
> depending on whether an error occurred.
Sure, but that's an argument for IS_ERR(), not the IS_ERR_OR_NULL() shite...
Al Viro <[email protected]> writes:
> [snip]
> Another part I really dislike in that area (not your fault, obviously)
> is
>
> void putname(struct filename *name)
> {
> if (IS_ERR_OR_NULL(name))
> return;
>
> in mainline right now. Could somebody explain when the hell has NULL
> become a possibility here? OK, I buy putname(ERR_PTR(...)) being
> a no-op, but IME every sodding time we mixed NULL and ERR_PTR() in
> an API we ended up with headache later.
>
> IS_ERR_OR_NULL() is almost always wrong. NULL as argument
> for destructor makes sense when constructor can fail with NULL;
> not the case here.
>
> How about the variant in vfs.git#misc.namei?
I went and looked through the changelog of fs/namei.c since this was
changed and don't see anything setting a filename NULL, so it seems safe
and good to me. I couldn't check *every* user of filename but the change
was only two months ago. Feel free to use my r-b for that commit if you
want.
Reviewed-by: Stephen Brennan <[email protected]>