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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87tuiwrrhn.fsf@stepbren-lnx.us.oracle.com>
Date:   Tue, 07 Sep 2021 14:43:48 -0700
From:   Stephen Brennan <stephen.s.brennan@...cle.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] namei: fix use-after-free and adjust calling
 conventions

Al Viro <viro@...iv.linux.org.uk> 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?