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

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

[Liangbin Lian]

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@gmail.com>
---
 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;
-- 
2.32.0 (Apple Git-132)

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

[Almaz Alexandrovich]

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@gmail.com>
     Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>

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@gmail.com>
> ---
>   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;

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

[练亮斌]

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@gmail.com>
>      Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
>
> 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@gmail.com>
> > ---
> >   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;

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

[Konstantin Komarov]

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@gmail.com>
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@gmail.com>
>>       Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
>>
>> 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@gmail.com>
>>> ---
>>>    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;

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

[练亮斌]

Hello.

If ntfs_file_inode_operations is ok for $Extend, that's fine with me.

Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
于2022年6月1日周三 00:34写道：
>
> 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@gmail.com>
> 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@gmail.com>
> >>       Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> >>
> >> 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@gmail.com>
> >>> ---
> >>>    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;