archive mirror
 help / color / mirror / Atom feed
From: Al Viro <>
To: OGAWA Hirofumi <>
Cc: "Pali Rohár" <>,,,
	"Theodore Y. Ts'o" <>,
	"Namjae Jeon" <>,
	"Gabriel Krisman Bertazi" <>
Subject: Re: vfat: Broken case-insensitive support for UTF-8
Date: Mon, 20 Jan 2020 07:30:40 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Mon, Jan 20, 2020 at 01:04:42PM +0900, OGAWA Hirofumi wrote:

> Also, not directly same issue though. There is related issue for
> case-insensitive. Even if we use some sort of internal wide char
> (e.g. in nls, 16bits), dcache is holding name in user's encode
> (e.g. utf8). So inefficient to convert cached name to wide char for each
> access.
> Relatively recent EXT4 case-insensitive may tackled this though, I'm not
> checking it yet.

What's more, comparisons in dcache lookups have to be very careful about
the rename-related issues.  You can give false negatives if the name
changes under you; it's not a problem.  You can even give a false positive
in case of name change happening in the middle of comparison; ->d_seq
mismatch will get caught and it will have the result discarded before it
causes problems.  However, you can't e.g. assume that the string you are
trying to convert from utf8 to 16bit won't be changing right under you.
Again, the wrong result of comparison in such situation is not a problem;
wrong return value is not the worst thing that can happen to a string
function mistakenly assuming that the string is not changing under it.

And you very much need to be careful about the things you can access
there.  E.g. something like "oh, I'll just look at the flags in the
inode of the parent of potential match" (as in the recently posted
series) is a bloody bad idea on many levels.  Starting with "your
potential match is getting moved right now, and what used to be its
parent becomes negative by the time you get around to fetching its
->d_inode.  Dereferencing the resulting NULL to get inode flags
is not pretty".

<checks ext4>
Yup, that bug is there as well, all right.  Look:
static int ext4_d_compare(const struct dentry *dentry, unsigned int len,
                          const char *str, const struct qstr *name)
        struct qstr qstr = {.name = str, .len = len };
        struct inode *inode = dentry->d_parent->d_inode;

        if (!IS_CASEFOLDED(inode) || !EXT4_SB(inode->i_sb)->s_encoding) {

Guess what happens if your (lockless) call of ->d_compare() runs
into the following sequence:
CPU1:	ext4_d_compare() fetches ->d_parent
CPU1:	takes a hardware interrupt
CPU2:	dentry gets evicted by memory pressure; so is its parent, since
it was the only thing that used to keep it pinned.  Eviction of the parent
calls dentry_unlink_inode() on the parent, which zeroes its ->d_inode.
CPU1:	comes back
CPU1:	fetches parent's ->d_inode and gets NULL
CPU1:	oopses on null pointer dereference.

It's not impossible to hit.  Note that e.g. vfat_cmpi() is not vulnerable
to that problem - ->d_sb is stable and both the superblock and ->nls_io
freeing is RCU-delayed.

I hadn't checked ->d_compare() instances for a while; somebody needs to
do that again, by the look of it.  The above definitely is broken;
no idea how many other instaces had grown such bugs...

  reply	other threads:[~2020-01-20  7:30 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-19 22:14 vfat: Broken case-insensitive support for UTF-8 Pali Rohár
2020-01-19 23:08 ` Al Viro
2020-01-19 23:33   ` Pali Rohár
2020-01-20  0:09     ` Al Viro
2020-01-20 11:19       ` Pali Rohár
2020-01-20  4:04 ` OGAWA Hirofumi
2020-01-20  7:30   ` Al Viro [this message]
2020-01-20  7:45     ` Al Viro
2020-01-20  8:07       ` oopsably broken case-insensitive support in ext4 and f2fs (Re: vfat: Broken case-insensitive support for UTF-8) Al Viro
2020-01-20 19:35         ` Al Viro
2020-01-24  4:29           ` Eric Biggers
2020-01-24 17:47             ` Linus Torvalds
2020-01-24 18:03               ` Jaegeuk Kim
2020-01-24 18:45                 ` Eric Biggers
2020-01-20 11:04   ` vfat: Broken case-insensitive support for UTF-8 Pali Rohár
2020-01-20 12:07     ` OGAWA Hirofumi
2020-01-20 21:40       ` Pali Rohár
2020-01-20 22:46         ` Al Viro
2020-01-20 23:57           ` Pali Rohár
2020-01-21  0:07             ` Al Viro
2020-01-21 20:34               ` Pali Rohár
2020-01-21 21:36                 ` Al Viro
2020-01-21 22:14                   ` Al Viro
2020-01-21 22:46                     ` Pali Rohár
2020-01-26 23:08                 ` Pali Rohár
2020-01-21 12:43             ` David Laight
2020-01-22  0:25         ` Gabriel Krisman Bertazi
2020-01-20 15:07     ` David Laight
2020-01-20 15:20       ` Pali Rohár
2020-01-20 15:47         ` David Laight
2020-01-20 16:12           ` Al Viro
2020-01-20 16:51             ` David Laight
2020-01-20 16:27           ` Pali Rohár
2020-01-20 16:43             ` David Laight
2020-01-20 16:56               ` Pali Rohár
2020-01-20 17:37       ` Theodore Y. Ts'o
2020-01-20 17:32   ` Theodore Y. Ts'o
2020-01-20 17:56     ` Pali Rohár
2020-01-21  3:52     ` OGAWA Hirofumi
2020-01-21 11:00       ` Pali Rohár
2020-01-21 12:26         ` OGAWA Hirofumi

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).