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 <[email protected]>
---
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)
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 <[email protected]>
Signed-off-by: Konstantin Komarov <[email protected]>
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 <[email protected]>
> ---
> 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;
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 <[email protected]>
> Signed-off-by: Konstantin Komarov <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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;
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 <[email protected]>
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 <[email protected]>
>> Signed-off-by: Konstantin Komarov <[email protected]>
>>
>> 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 <[email protected]>
>>> ---
>>> 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;
Hello.
If ntfs_file_inode_operations is ok for $Extend, that's fine with me.
Konstantin Komarov <[email protected]>
于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 <[email protected]>
> 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 <[email protected]>
> >> Signed-off-by: Konstantin Komarov <[email protected]>
> >>
> >> 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 <[email protected]>
> >>> ---
> >>> 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;