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?
