linux-kernel - Re: [PATCH] fs/ntfs3: fix null pointer dereference in d_flags_for

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e40981eb-8243-4bb1-44ef-5393cc464c03@paragon-software.com>
Date:   Tue, 31 May 2022 19:34:19 +0300
From:   Konstantin Komarov <almaz.alexandrovich@...agon-software.com>
To:     练亮斌 <jjm2473@...il.com>
CC:     <ntfs3@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fs/ntfs3: fix null pointer dereference in
 d_flags_for_inode

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