Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Stephen Brennan <stephen.s.brennan@oracle.com>
To:     Al Viro <viro@zeniv.linux.org.uk>
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
In-Reply-To: <YTfVE7IbbTV71Own@zeniv-ca.linux.org.uk>
References: <20210901175144.121048-1-stephen.s.brennan@oracle.com>
 <YTfVE7IbbTV71Own@zeniv-ca.linux.org.uk>
Date:   Tue, 07 Sep 2021 14:43:48 -0700
Message-ID: <87tuiwrrhn.fsf@stepbren-lnx.us.oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?H83BeF2Z6lf25z+aiHSQN6xd3sLGuPt8dBA3zjpEdeId2zxcMwXv5mzVrz5B?=
 =?us-ascii?Q?K12sshN81ZLsJ3V5yseDsoccJ9vUaniIxKsA1C3EsMA581P8Bv6vlDtwQTaN?=
 =?us-ascii?Q?aQguBgI2+KyzoDqV3CpJuuAMvNZHB1XX9/kiuGkdqFOKDu44s1b9kXV1B6AY?=
 =?us-ascii?Q?/il6VBJFsH6caeo5OOvMwZIRa1WJWH99Q75pwxUsX0i9b2FTGdkqaBWrQ07e?=
 =?us-ascii?Q?deIGdWLQ8yoLQATq+6QDkAyLYssM0v3PjdqZMnzXorl/w8/0QDGYSGkmJ8a7?=
 =?us-ascii?Q?/+62ZoAmgcsuR408rr56e+SlapE7/waUAxU8rls6X5lpG08UquP/D5ogrRZF?=
 =?us-ascii?Q?zYEV7Dt/o2gEN22rPIZE8VMBFujoCsihbUKFwNnlxIfgps5K3yXOMZryCfbX?=
 =?us-ascii?Q?9IUBCiCdmhgCFUAohCp/in+AqZGV40Q/feDz2cQbrYXwAhpRpfh/VOGhboaQ?=
 =?us-ascii?Q?/0f4Kr0I1mnIHzFuliXXarEhG6tW+mfFFutdvAOaUbILgeb84pyp2p7HJZ1J?=
 =?us-ascii?Q?jZCLOQ8mzuiMAyQqMpyTP7cMr/9aZVWxrfzGi80Qwv6464qmHOiqRfZwVK/d?=
 =?us-ascii?Q?u80NSk/FPTN4egTDe43MjikzYcAAsHKL448Qp5syQTTI4MnrVdZ+hRi9E5RU?=
 =?us-ascii?Q?npvEwdhUS7MVIhzMwcQLt31FgkrxSZ3w62QibVtlMqp5ahM5uk5iEJMn+dwb?=
 =?us-ascii?Q?rAtWEuE3X7BJqKxTWx4wyyfeCADJOumnnBXKy45F8P2QXnTOfXSR9MSCKAf3?=
 =?us-ascii?Q?F6fObGH9q3fDBpQchnrs3DI3kqJtJTuzigoUIGBPvfcKtKE7zmyGmpphpD0m?=
 =?us-ascii?Q?pcbvyleBSdbw9g7Xvh8NQ+4PwrvKEAZdWz98ZRNesfkPFcboWqhfxaOhhBIf?=
 =?us-ascii?Q?7z8aSAk34YFzL46/a+6jlU7M1NQvRAKO1VSSqkmGsJhizl/pIOZyyqiBDLBv?=
 =?us-ascii?Q?Tq4wknOefURWtHNbcSkleGHaBa3FEM91zdP7WgguV8a9U54xNHCBNvwOd0Kw?=
 =?us-ascii?Q?2xyqCpBFLWw4P4uffptkl+E8xIxzFLqx9xQYTFg/ZJ2e0cfJYC8sy/yKaCwY?=
 =?us-ascii?Q?VMrLHqe1L8pUIQt8KS1uBi4wD1eQdGlRIVebNqTnSJtwZisVugnoRd6pfZYf?=
 =?us-ascii?Q?rwdHlqL26M5xTCcKoTH90zf+zVSJ3ZMtBzGtOUCFRa4QqrojeXHxPKPAetGv?=
 =?us-ascii?Q?YmrzhLe5HEMMNnDEUw3rLSULoBYyIkEZCl22GUjEjbmwgEXyGDP5zWTjhiLc?=
 =?us-ascii?Q?9nNo3wgSmJMy/jnOG1p48dlx3qMcHnYUQYz2tStkoLYhh/W15Kw/68aOCQxe?=
 =?us-ascii?Q?4pyE7bmZisUzwcr3gIcoUVos?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 37d22517-8e97-458b-f6ab-08d9724893f0
X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4166.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Sep 2021 21:43:49.6040
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CYdvkRGl4/+ww7KeP9/peN1hrMPaOVbjHgRKuJAWo+QB7AT78Hc9JrGVyHRoMQgTzdizCwfQD7wTja0k54fnsTK7vXnuTmeaOc0wS9ZRmu4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB4908
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10100 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0
 malwarescore=0 spamscore=0 mlxscore=0 phishscore=0 mlxlogscore=999
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109030001 definitions=main-2109070136
X-Proofpoint-GUID: O78bjq5un3PFko3HvcTy67UAeeeJrRwE
X-Proofpoint-ORIG-GUID: O78bjq5un3PFko3HvcTy67UAeeeJrRwE
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Al Viro <viro@zeniv.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?