From: Gabriel Krisman Bertazi <krisman@collabora.com> To: Eric Biggers <ebiggers@kernel.org> Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, stable@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>, Daniel Rosenberg <drosen@google.com>, Gabriel Krisman Bertazi <krisman@collabora.co.uk> Subject: Re: [PATCH] ext4: avoid utf8_strncasecmp() with unstable name Date: Sat, 30 May 2020 02:17:02 -0400 [thread overview] Message-ID: <85d06mkkv5.fsf@collabora.com> (raw) In-Reply-To: <20200530060216.221456-1-ebiggers@kernel.org> (Eric Biggers's message of "Fri, 29 May 2020 23:02:16 -0700") Eric Biggers <ebiggers@kernel.org> writes: > From: Eric Biggers <ebiggers@google.com> > > 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: <stable@vger.kernel.org> # v5.2+ > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Daniel Rosenberg <drosen@google.com> > Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk> > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > 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
WARNING: multiple messages have this Message-ID (diff)
From: Gabriel Krisman Bertazi <krisman@collabora.com> To: Eric Biggers <ebiggers@kernel.org> Cc: Daniel Rosenberg <drosen@google.com>, stable@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, Al Viro <viro@zeniv.linux.org.uk>, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, Gabriel Krisman Bertazi <krisman@collabora.co.uk> Subject: Re: [f2fs-dev] [PATCH] ext4: avoid utf8_strncasecmp() with unstable name Date: Sat, 30 May 2020 02:17:02 -0400 [thread overview] Message-ID: <85d06mkkv5.fsf@collabora.com> (raw) In-Reply-To: <20200530060216.221456-1-ebiggers@kernel.org> (Eric Biggers's message of "Fri, 29 May 2020 23:02:16 -0700") Eric Biggers <ebiggers@kernel.org> writes: > From: Eric Biggers <ebiggers@google.com> > > 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: <stable@vger.kernel.org> # v5.2+ > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Daniel Rosenberg <drosen@google.com> > Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk> > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > 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 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2020-05-30 6:17 UTC|newest] Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-30 6:02 [PATCH] ext4: avoid utf8_strncasecmp() with unstable name Eric Biggers 2020-05-30 6:02 ` [f2fs-dev] " Eric Biggers 2020-05-30 6:17 ` Gabriel Krisman Bertazi [this message] 2020-05-30 6:17 ` Gabriel Krisman Bertazi 2020-05-30 6:44 ` Eric Biggers 2020-05-30 6:44 ` [f2fs-dev] " Eric Biggers 2020-05-30 6:52 ` Gabriel Krisman Bertazi 2020-05-30 17:18 ` Matthew Wilcox 2020-05-30 17:18 ` [f2fs-dev] " Matthew Wilcox 2020-05-30 17:35 ` Eric Biggers 2020-05-30 17:35 ` [f2fs-dev] " Eric Biggers 2020-05-30 17:59 ` Al Viro 2020-05-30 17:59 ` [f2fs-dev] " Al Viro 2020-06-01 6:45 ` Eric Biggers 2020-06-01 6:45 ` [f2fs-dev] " Eric Biggers 2020-05-30 20:41 ` Matthew Wilcox 2020-05-30 20:41 ` [f2fs-dev] " Matthew Wilcox 2020-06-01 7:05 ` Eric Biggers 2020-06-01 7:05 ` [f2fs-dev] " Eric Biggers
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=85d06mkkv5.fsf@collabora.com \ --to=krisman@collabora.com \ --cc=drosen@google.com \ --cc=ebiggers@kernel.org \ --cc=krisman@collabora.co.uk \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-fsdevel@vger.kernel.org \ --cc=stable@vger.kernel.org \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.