[0/3] namei: fix use-after-free and adjust calling conventions

Message

Stephen Brennan Sept. 1, 2021, 5:51 p.m. UTC

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(-)

Comments

Al Viro Sept. 7, 2021, 9:09 p.m. UTC | #1

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?

Stephen Brennan Sept. 7, 2021, 9:43 p.m. UTC | #2

Al Viro <[e-mail]> 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=[e-mail]/

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?

Al Viro Sept. 7, 2021, 9:54 p.m. UTC | #3

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...

Stephen Brennan Sept. 8, 2021, 6:47 p.m. UTC | #4

Al Viro <[e-mail]> 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 <[e-mail]>