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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 7 Sep 2021 21:09:39 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Stephen Brennan <stephen.s.brennan@...cle.com>
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

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?