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=-6.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, UNPARSEABLE_RELAY autolearn=ham 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 6BCB1C433E0 for ; Sat, 30 May 2020 06:17:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 46D262073B for ; Sat, 30 May 2020 06:17:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728762AbgE3GRL (ORCPT ); Sat, 30 May 2020 02:17:11 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:47558 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728657AbgE3GRL (ORCPT ); Sat, 30 May 2020 02:17:11 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: krisman) with ESMTPSA id 121BA2A49A0 From: Gabriel Krisman Bertazi To: Eric Biggers Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, stable@vger.kernel.org, Al Viro , Daniel Rosenberg , Gabriel Krisman Bertazi Subject: Re: [PATCH] ext4: avoid utf8_strncasecmp() with unstable name Organization: Collabora References: <20200530060216.221456-1-ebiggers@kernel.org> Date: Sat, 30 May 2020 02:17:02 -0400 In-Reply-To: <20200530060216.221456-1-ebiggers@kernel.org> (Eric Biggers's message of "Fri, 29 May 2020 23:02:16 -0700") Message-ID: <85d06mkkv5.fsf@collabora.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org Eric Biggers writes: > From: Eric Biggers > > If the dentry name passed to ->d_compare() fits in dentry::d_iname, then > it may be concurrently modified by a rename. This can cause undefined > behavior (possibly out-of-bounds memory accesses or crashes) in > utf8_strncasecmp(), since fs/unicode/ isn't written to handle strings > that may be concurrently modified. > > Fix this by first copying the filename to a stack buffer if needed. > This way we get a stable snapshot of the filename. > > Fixes: b886ee3e778e ("ext4: Support case-insensitive file name lookups") > Cc: # v5.2+ > Cc: Al Viro > Cc: Daniel Rosenberg > Cc: Gabriel Krisman Bertazi > Signed-off-by: Eric Biggers > --- > fs/ext4/dir.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c > index c654205f648dd..19aef8328bb18 100644 > --- a/fs/ext4/dir.c > +++ b/fs/ext4/dir.c > @@ -675,6 +675,7 @@ static int ext4_d_compare(const struct dentry *dentry, unsigned int len, > struct qstr qstr = {.name = str, .len = len }; > const struct dentry *parent = READ_ONCE(dentry->d_parent); > const struct inode *inode = READ_ONCE(parent->d_inode); > + char strbuf[DNAME_INLINE_LEN]; > > if (!inode || !IS_CASEFOLDED(inode) || > !EXT4_SB(inode->i_sb)->s_encoding) { > @@ -683,6 +684,22 @@ static int ext4_d_compare(const struct dentry *dentry, unsigned int len, > return memcmp(str, name->name, len); > } > > + /* > + * If the dentry name is stored in-line, then it may be concurrently > + * modified by a rename. If this happens, the VFS will eventually retry > + * the lookup, so it doesn't matter what ->d_compare() returns. > + * However, it's unsafe to call utf8_strncasecmp() with an unstable > + * string. Therefore, we have to copy the name into a temporary buffer. > + */ > + if (len <= DNAME_INLINE_LEN - 1) { > + unsigned int i; > + > + for (i = 0; i < len; i++) > + strbuf[i] = READ_ONCE(str[i]); > + strbuf[len] = 0; > + qstr.name = strbuf; > + } > + Could we avoid this if the casefolded version were cached in the dentry? Then we could use utf8_strncasecmp_folded which would be safe. Would this be acceptable for vfs? > return ext4_ci_compare(inode, name, &qstr, false); > } -- Gabriel Krisman Bertazi