Re: [PATCH V4] fs/ntfs3: check if the inode is bad before creating symlink

[Jan Kara <jack@suse.cz>]

On Fri 22-11-24 16:10:25, Lizhi Xu wrote:
> syzbot reported a null-ptr-deref in pick_link. [1]
> 
> First, i_link and i_dir_seq are in the same union, they share the same memory
> address, and i_dir_seq will be updated during the execution of walk_component,
> which makes the value of i_link equal to i_dir_seq.
> 
> Secondly, the chmod execution failed, which resulted in setting the mode value
> of file0's inode to REG when executing ntfs_bad_inode.
> 
> Third, during the execution of the link command, it sets the inode of the
> symlink file to the already bad inode of file0 by calling d_instantiate, which
> ultimately leads to null-ptr-deref when performing a mount operation on the
> symbolic link bus because it use bad inode's i_link and its value is equal to
> i_dir_seq=2. 
> 
> Note: ("file0, bus" are defined in reproducer [2])
> 
> To avoid null-ptr-deref in pick_link, when creating a symbolic link, first check
> whether the inode of file is already bad.

So actually there's no symbolic link involved here at all (which what was
confusing me all the time).

> move_mount(0xffffffffffffff9c, &(0x7f00000003c0)='./file0\x00', 0xffffffffffffff9c, &(0x7f0000000400)='./file0/file0\x00', 0x140)
> chmod(&(0x7f0000000080)='./file0\x00', 0x0)
> link(&(0x7f0000000200)='./file0\x00', &(0x7f0000000240)='./bus\x00')
> mount$overlay(0x0, &(0x7f00000000c0)='./bus\x00', 0x0, 0x0, 0x0)

This creates only a hardlink. And in fact the creation of the link seems to
be totally irrelevant for this problem. I believe:

move_mount(0xffffffffffffff9c, &(0x7f00000003c0)='./file0\x00', 0xffffffffffffff9c, &(0x7f0000000400)='./file0/file0\x00', 0x140)
chmod(&(0x7f0000000080)='./file0\x00', 0x0)
mount$overlay(0x0, &(0x7f00000000c0)='./file0\x00', 0x0, 0x0, 0x0)

would be as good reproducer of the problem. The core of the problem is that
NTFS3 calls make_bad_inode() on inode that is accessible to userspace and
is something else than a regular file. As long as that happens, some
variant of this NULL-ptr-dereference can happen as well, just the
reproducers will be somewhat different.

So I don't think patching ntfs_link_inode() makes a lot of sense. If
anything, I'd patch NTFS3 to not mark the inode as bad somewhere inside
ntfs_setattr() and deal with the error in a better way.

								Honza

> 
> Reported-by: syzbot+73d8fc29ec7cba8286fa@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=73d8fc29ec7cba8286fa
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
> V1 --> V2: add the root cause of the i_link not set issue and imporve the check
> V2 --> V3: when creating a symbolic link, first check whether the inode of file is bad.
> V3 --> V4: add comments for symlink use bad inode, it is the root cause
> 
>  fs/ntfs3/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index be04d2845bb7..fefbdcf75016 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -1719,6 +1719,9 @@ int ntfs_link_inode(struct inode *inode, struct dentry *dentry)
>  	struct ntfs_sb_info *sbi = inode->i_sb->s_fs_info;
>  	struct NTFS_DE *de;
>  
> +	if (is_bad_inode(inode))
> +		return -EIO;
> +
>  	/* Allocate PATH_MAX bytes. */
>  	de = __getname();
>  	if (!de)
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR