Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/
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?
