linux-kernel - Re: [PATCH] fs: Prevent non-symlinks from entering pick link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250618061815.GR1880847@ZenIV>
Date: Wed, 18 Jun 2025 07:18:15 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Edward Adam Davis <eadavis@...com>
Cc: almaz.alexandrovich@...agon-software.com, brauner@...nel.org,
	jack@...e.cz, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, ntfs3@...ts.linux.dev,
	syzbot+1aa90f0eb1fc3e77d969@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] fs: Prevent non-symlinks from entering pick link

On Wed, Jun 18, 2025 at 01:34:18PM +0800, Edward Adam Davis wrote:
> On Wed, 18 Jun 2025 06:27:47 +0100, Al Viro wrote:
> > Note that anything that calls __d_add(dentry, inode) with is_bad_inode(inode)
> > (or d_add(), or d_instantiate(), or d_splice_alias() under the same conditions)
> > is also FUBAR.
> > 
> > So's anything that calls make_bad_inode() on a struct inode that might be
> > in process of being passed to one of those functions by another thread.
> > 
> > This is fundamentally wrong; bad inodes are not supposed to end up attached
> > to dentries.
> As far as I know, pick_link() is used to resolve the target path of a
> symbolic link (symlink). Can you explain why pick_link() is executed on
> a directory or a regular file?

Because the inode_operations of that thing contains ->get_link().  Which means
"symlink" to dcache.  Again, there is code all over the place written in
assumption that no dentry will ever have ->d_inode pointing to any of those.

No, we are not going to paper over that in __d_add() or __d_instantiate() either;
it's fundamentally a losing game.  _Maybe_ a couple of WARN_ON() when built with
CONFIG_DEBUG_VFS or something similar, but that would only make for slightly
more specific diagnostics; not all that useful, since you can literally grep for
_ntfs_bad_inode to pick the location of actual underlying bugs.

Again, the underlying bug is that make_bad_inode() is called on a live inode.
In some cases it's "icache lookup finds a normal inode, d_splice_alias() is called
to attach it to dentry, while another thread decides to call make_bad_inode() on
it - that would evict it from icache, but we'd already found it there earlier".
In some it's outright "we have an inode attached to dentry - that's how we got
it in the first place; let's call make_bad_inode() on it just for shits and giggles".
Either is a bug.

_ntfs_bad_inode() uses are completely broken.  Matter of fact, we probably ought to
retire make_bad_inode() - there are few callers and most of them don't actually
need anything other than remove_inode_hash() (e.g. iget_failed()).  In any case,
whether there is a case for several new helpers or not, the kind of use
_ntfs_bad_inode() gets is right out.