Re: [PATCH] fs/ntfs3: fix null pointer dereference in d_flags_for_inode

[Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>]

Hello.

In the end of ntfs_read_mft function we must assign correct i_op:
inode->i_op = &ntfs_dir_inode_operations;
<...>
inode->i_op = &ntfs_link_inode_operations;
<...>
inode->i_op = &ntfs_file_inode_operations;
<...>
inode->i_op = &ntfs_special_inode_operations;

In this if .. else if .. else is an error:
records in $Extend doesn't get correct i_op.
This was fixed in my patch.

Line in beginning of ntfs_read_mft function
inode->i_op = NULL;
triggered null pointer dereference because
inode->i_op = &ntfs_file_inode_operations;
is missing for records in $Extend.

If I just remove inode->i_op = NULL,
then in i_op will be some previous value.
Sometimes this value is correct, sometimes it's not.

I'm thankful, that you've spent time to find and debug this issue.
This was reflected in line Reported-by: Liangbin Lian <jjm2473@xxxxxxxxx>
I hope I've made more clear my previous message.


On 5/28/22 16:42, 练亮斌 wrote:
> Hello.
> `inode->i_op` already initialized when inode alloc, this bug was
> introduced by `inode->i_op = NULL;`, just delete this line.
> Please check my patch, maybe it's a better one, I have tested it on my project.
>
> On 5/26/22 18:23, Almaz Alexandrovich wrote:
> > Hello.
> >
> > Thank you for reporting this bug.
> > The bug happens because we don't initialize i_op for records in $Extend.
> > We tested patch on our side, let me know if patch helps you too.
> >
> >       fs/ntfs3: Fix missing i_op in ntfs_read_mft
> >
> >       There is null pointer dereference because i_op == NULL.
> >       The bug happens because we don't initialize i_op for records in $Extend.
> >       Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> >
> >       Reported-by: Liangbin Lian <jjm2473@xxxxxxxxx>
> >       Signed-off-by: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> >
> > diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> > index 879952254071..b2cc1191be69 100644
> > --- a/fs/ntfs3/inode.c
> > +++ b/fs/ntfs3/inode.c
> > @@ -430,6 +430,7 @@ static struct inode *ntfs_read_mft(struct inode *inode,
> >           } else if (fname && fname->home.low == cpu_to_le32(MFT_REC_EXTEND) &&
> >                      fname->home.seq == cpu_to_le16(MFT_REC_EXTEND)) {
> >                   /* Records in $Extend are not a files or general directories. */
> > +               inode->i_op = &ntfs_file_inode_operations;
> >           } else {
> >                   err = -EINVAL;
> >                   goto out;
> >
> >
> > On 5/6/22 06:46, Liangbin Lian wrote:
> > > ntfs_read_mft may return inode with null i_op, cause null pointer dereference in d_flags_for_inode (inode->i_op->get_link).
> > > Reproduce:
> > >    - sudo mount -t ntfs3 -o loop ntfs.img ntfs
> > >    - ls ntfs/'$Extend/$Quota'
> > >
> > > The call trace is shown below (striped):
> > >    BUG: kernel NULL pointer dereference, address: 0000000000000008
> > >    CPU: 0 PID: 577 Comm: ls Tainted: G           OE     5.16.0-0.bpo.4-amd64 #1  Debian 5.16.12-1~bpo11+1
> > >    RIP: 0010:d_flags_for_inode+0x65/0x90
> > >    Call Trace:
> > >    ntfs_lookup
> > >    +--- dir_search_u
> > >    |    +--- ntfs_iget5
> > >    |         +--- ntfs_read_mft
> > >    +--- d_splice_alias
> > >         +--- __d_add
> > >              +--- d_flags_for_inode
> > >
> > > Signed-off-by: Liangbin Lian <jjm2473@xxxxxxxxx>
> > > ---
> > >    fs/ntfs3/inode.c | 1 -
> > >    1 file changed, 1 deletion(-)
> > >
> > > diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> > > index 9eab11e3b..b68d26fa8 100644
> > > --- a/fs/ntfs3/inode.c
> > > +++ b/fs/ntfs3/inode.c
> > > @@ -45,7 +45,6 @@ static struct inode *ntfs_read_mft(struct inode *inode,
> > >        struct MFT_REC *rec;
> > >        struct runs_tree *run;
> > >
> > > -     inode->i_op = NULL;
> > >        /* Setup 'uid' and 'gid' */
> > >        inode->i_uid = sbi->options->fs_uid;
> > >        inode->i_gid = sbi->options->fs_gid;