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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=73d8fc29ec7cba8286fa
> Signed-off-by: Lizhi Xu <lizhi.xu@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxx>
SUSE Labs, CR
