linux-kernel - Re: [PATCH for 6.19-rc1] fs: preserve file type in make_bad

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: <wqkxevwtev5p77czk2com5zvbbwcpxxeucrt7zbgjciqxjyivx@c7624klburuh>
Date: Wed, 10 Dec 2025 11:09:12 +0100
From: Jan Kara <jack@...e.cz>
To: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: Mateusz Guzik <mjguzik@...il.com>, Al Viro <viro@...iv.linux.org.uk>, 
	syzbot <syzbot+d222f4b7129379c3d5bc@...kaller.appspotmail.com>, brauner@...nel.org, jack@...e.cz, jlbec@...lplan.org, 
	joseph.qi@...ux.alibaba.com, linkinjeon@...nel.org, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, mark@...heh.com, ocfs2-devel@...ts.linux.dev, 
	sj1557.seo@...sung.com, syzkaller-bugs@...glegroups.com, 
	Chuck Lever <chuck.lever@...cle.com>
Subject: Re: [PATCH for 6.19-rc1] fs: preserve file type in make_bad_inode()
 unless invalid

On Wed 10-12-25 18:45:26, Tetsuo Handa wrote:
> syzbot is hitting VFS_BUG_ON_INODE(!S_ISDIR(inode->i_mode)) check
> introduced by commit e631df89cd5d ("fs: speed up path lookup with cheaper
> handling of MAY_EXEC"), for make_bad_inode() is blindly changing file type
> to S_IFREG. Since make_bad_inode() might be called after an inode is fully
> constructed, make_bad_inode() should not needlessly change file type.
> 
> Reported-by: syzbot+d222f4b7129379c3d5bc@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d222f4b7129379c3d5bc
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>

No. make_bad_inode() must not be called once the inode is fully visible
because that can cause all sorts of fun. That function is really only good
for handling a situation when read of an inode from the disk failed or
similar early error paths. It would be great if make_bad_inode() could do
something like:

	VFS_BUG_ON_INODE(!(inode_state_read_once(inode) & I_NEW));

but sadly that is not currently possible because inodes start with i_state
set to 0 and some places do call make_bad_inode() before I_NEW is set in
i_state. Matheusz wanted to clean that up a bit AFAIK.

Until the cleanup is done, perhaps we could add:

	VFS_BUG_ON_INODE(inode->i_dentry->first);

to make_bad_inode() and watch the fireworks from syzbot. But at least the
bugs would be attributed to the place where they are happening.

								Honza

> ---
> Should we implement all callbacks (except get_offset_ctx callback which is
> currently used by only tmpfs which does not call make_bad_inode()) within
> bad_inode_ops, for there might be a callback which is expected to be non-NULL
> for !S_IFREG types? Implementing missing callbacks is good for eliminating
> possibility of NULL function pointer call. Since VFS is using
> 
>     if (!inode->i_op->foo)
>         return error;
>     inode->i_op->foo();
> 
> pattern instead of
> 
>     pFoo = READ_ONCE(inode->i_op->foo)
>     if (!pFoo)
>         return error;
>     pFoo();
> 
> pattern, suddenly replacing "one i_op with i_op->foo != NULL" with "another
> i_op with i_op->foo == NULL" has possibility of NULL pointer function call
> (e.g. https://lkml.kernel.org/r/18a58415-4aa9-4cba-97d2-b70384407313@I-love.SAKURA.ne.jp ).
> If we implement missing callbacks, e.g. vfs_fileattr_get() will start
> calling security_inode_file_getattr() on bad inode, but we can eliminate
> possibility of inode->i_op->fileattr_get == NULL when make_bad_inode() is
> called from security_inode_file_getattr() for some reason.
> 
>  fs/bad_inode.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bad_inode.c b/fs/bad_inode.c
> index 0ef9bcb744dd..ff6c2daecd1c 100644
> --- a/fs/bad_inode.c
> +++ b/fs/bad_inode.c
> @@ -207,7 +207,19 @@ void make_bad_inode(struct inode *inode)
>  {
>  	remove_inode_hash(inode);
>  
> -	inode->i_mode = S_IFREG;
> +	switch (inode->i_mode & S_IFMT) {
> +	case S_IFREG:
> +	case S_IFDIR:
> +	case S_IFLNK:
> +	case S_IFCHR:
> +	case S_IFBLK:
> +	case S_IFIFO:
> +	case S_IFSOCK:
> +		inode->i_mode &= S_IFMT;
> +		break;
> +	default:
> +		inode->i_mode = S_IFREG;
> +	}
>  	simple_inode_init_ts(inode);
>  	inode->i_op = &bad_inode_ops;	
>  	inode->i_opflags &= ~IOP_XATTR;
> -- 
> 2.47.3
> 
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR