From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5353EC433F5 for ; Tue, 7 Sep 2021 21:09:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2E24961103 for ; Tue, 7 Sep 2021 21:09:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344670AbhIGVKs (ORCPT ); Tue, 7 Sep 2021 17:10:48 -0400 Received: from zeniv-ca.linux.org.uk ([142.44.231.140]:49372 "EHLO zeniv-ca.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344715AbhIGVKr (ORCPT ); Tue, 7 Sep 2021 17:10:47 -0400 Received: from viro by zeniv-ca.linux.org.uk with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1mNiLb-0027k6-FO; Tue, 07 Sep 2021 21:09:39 +0000 Date: Tue, 7 Sep 2021 21:09:39 +0000 From: Al Viro To: Stephen Brennan Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/3] namei: fix use-after-free and adjust calling conventions Message-ID: References: <20210901175144.121048-1-stephen.s.brennan@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210901175144.121048-1-stephen.s.brennan@oracle.com> Sender: Al Viro Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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?