[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241122115050.7i3eslwb77tee37j@quack3>
Date: Fri, 22 Nov 2024 12:50:50 +0100
From: Jan Kara <jack@...e.cz>
To: Lizhi Xu <lizhi.xu@...driver.com>
Cc: viro@...iv.linux.org.uk, 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+73d8fc29ec7cba8286fa@...kaller.appspotmail.com,
syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH V4] fs/ntfs3: check if the inode is bad before creating
symlink
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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=73d8fc29ec7cba8286fa
> Signed-off-by: Lizhi Xu <lizhi.xu@...driver.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@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists