[PATCH 1/2] ntfs3: fix NULL deref in ntfs_update_mftmirr

[PATCH 1/2] ntfs3: fix NULL deref in ntfs_update_mftmirr

[Pavel Skripkin]

If ntfs_fill_super() wasn't called then sbi->sb will be equal to NULL.
Code should check this ptr before dereferencing. Syzbot hit this issue
via passing wrong mount param as can be seen from log below

Fail log:
ntfs3: Unknown parameter 'iochvrset'
general protection fault, probably for non-canonical address 0xdffffc0000000003: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f]
CPU: 1 PID: 3589 Comm: syz-executor210 Not tainted 5.18.0-rc3-syzkaller-00016-gb253435746d9 #0
...
Call Trace:
 <TASK>
 put_ntfs+0x1ed/0x2a0 fs/ntfs3/super.c:463
 ntfs_fs_free+0x6a/0xe0 fs/ntfs3/super.c:1363
 put_fs_context+0x119/0x7a0 fs/fs_context.c:469
 do_new_mount+0x2b4/0xad0 fs/namespace.c:3044
 do_mount fs/namespace.c:3383 [inline]
 __do_sys_mount fs/namespace.c:3591 [inline]

Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
Reported-and-tested-by: syzbot+c95173762127ad76a824@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 fs/ntfs3/fsntfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index 3de5700a9b83..891125ca6848 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -831,10 +831,15 @@ int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait)
 {
 	int err;
 	struct super_block *sb = sbi->sb;
-	u32 blocksize = sb->s_blocksize;
+	u32 blocksize;
 	sector_t block1, block2;
 	u32 bytes;
 
+	if (!sb)
+		return -EINVAL;
+
+	blocksize = sb->s_blocksize;
+
 	if (!(sbi->flags & NTFS_FLAGS_MFTMIRR))
 		return 0;
 
-- 
2.35.1

[PATCH 2/2] ntfs3: make ntfs_update_mftmirr return void

[Pavel Skripkin]

None of callers check the return value of ntfs_update_mftmirr(), so make
it return void to make code simpler.

Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 fs/ntfs3/fsntfs.c  | 20 +++++++-------------
 fs/ntfs3/ntfs_fs.h |  2 +-
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index 891125ca6848..938acb246b58 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -827,7 +827,7 @@ int ntfs_refresh_zone(struct ntfs_sb_info *sbi)
 /*
  * ntfs_update_mftmirr - Update $MFTMirr data.
  */
-int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait)
+void ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait)
 {
 	int err;
 	struct super_block *sb = sbi->sb;
@@ -836,12 +836,12 @@ int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait)
 	u32 bytes;
 
 	if (!sb)
-		return -EINVAL;
+		return;
 
 	blocksize = sb->s_blocksize;
 
 	if (!(sbi->flags & NTFS_FLAGS_MFTMIRR))
-		return 0;
+		return;
 
 	err = 0;
 	bytes = sbi->mft.recs_mirr << sbi->record_bits;
@@ -852,16 +852,13 @@ int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait)
 		struct buffer_head *bh1, *bh2;
 
 		bh1 = sb_bread(sb, block1++);
-		if (!bh1) {
-			err = -EIO;
-			goto out;
-		}
+		if (!bh1)
+			return;
 
 		bh2 = sb_getblk(sb, block2++);
 		if (!bh2) {
 			put_bh(bh1);
-			err = -EIO;
-			goto out;
+			return;
 		}
 
 		if (buffer_locked(bh2))
@@ -881,13 +878,10 @@ int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait)
 
 		put_bh(bh2);
 		if (err)
-			goto out;
+			return;
 	}
 
 	sbi->flags &= ~NTFS_FLAGS_MFTMIRR;
-
-out:
-	return err;
 }
 
 /*
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index fb825059d488..061dafbdcedd 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -592,7 +592,7 @@ int ntfs_look_free_mft(struct ntfs_sb_info *sbi, CLST *rno, bool mft,
 void ntfs_mark_rec_free(struct ntfs_sb_info *sbi, CLST rno);
 int ntfs_clear_mft_tail(struct ntfs_sb_info *sbi, size_t from, size_t to);
 int ntfs_refresh_zone(struct ntfs_sb_info *sbi);
-int ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait);
+void ntfs_update_mftmirr(struct ntfs_sb_info *sbi, int wait);
 enum NTFS_DIRTY_FLAGS {
 	NTFS_DIRTY_CLEAR = 0,
 	NTFS_DIRTY_DIRTY = 1,
-- 
2.35.1

Re: [PATCH 1/2] ntfs3: fix NULL deref in ntfs_update_mftmirr

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 1184 bytes --]

On 4/21/22 23:53, Pavel Skripkin wrote:
> If ntfs_fill_super() wasn't called then sbi->sb will be equal to NULL.
> Code should check this ptr before dereferencing. Syzbot hit this issue
> via passing wrong mount param as can be seen from log below
> 
> Fail log:
> ntfs3: Unknown parameter 'iochvrset'
> general protection fault, probably for non-canonical address 0xdffffc0000000003: 0000 [#1] PREEMPT SMP KASAN
> KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f]
> CPU: 1 PID: 3589 Comm: syz-executor210 Not tainted 5.18.0-rc3-syzkaller-00016-gb253435746d9 #0
> ...
> Call Trace:
>   <TASK>
>   put_ntfs+0x1ed/0x2a0 fs/ntfs3/super.c:463
>   ntfs_fs_free+0x6a/0xe0 fs/ntfs3/super.c:1363
>   put_fs_context+0x119/0x7a0 fs/fs_context.c:469
>   do_new_mount+0x2b4/0xad0 fs/namespace.c:3044
>   do_mount fs/namespace.c:3383 [inline]
>   __do_sys_mount fs/namespace.c:3591 [inline]
> 
> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> Reported-and-tested-by: syzbot+c95173762127ad76a824@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>

gentle ping




With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] ntfs3: fix NULL deref in ntfs_update_mftmirr

[Konstantin Komarov]

On 6/4/22 14:42, Pavel Skripkin wrote:
> On 4/21/22 23:53, Pavel Skripkin wrote:
>> If ntfs_fill_super() wasn't called then sbi->sb will be equal to NULL.
>> Code should check this ptr before dereferencing. Syzbot hit this issue
>> via passing wrong mount param as can be seen from log below
>>
>> Fail log:
>> ntfs3: Unknown parameter 'iochvrset'
>> general protection fault, probably for non-canonical address 0xdffffc0000000003: 0000 [#1] PREEMPT SMP KASAN
>> KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f]
>> CPU: 1 PID: 3589 Comm: syz-executor210 Not tainted 5.18.0-rc3-syzkaller-00016-gb253435746d9 #0
>> ...
>> Call Trace:
>>   <TASK>
>>   put_ntfs+0x1ed/0x2a0 fs/ntfs3/super.c:463
>>   ntfs_fs_free+0x6a/0xe0 fs/ntfs3/super.c:1363
>>   put_fs_context+0x119/0x7a0 fs/fs_context.c:469
>>   do_new_mount+0x2b4/0xad0 fs/namespace.c:3044
>>   do_mount fs/namespace.c:3383 [inline]
>>   __do_sys_mount fs/namespace.c:3591 [inline]
>>
>> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
>> Reported-and-tested-by: syzbot+c95173762127ad76a824@syzkaller.appspotmail.com
>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> 
> gentle ping
> 
> 
> 
> 
> With regards,
> Pavel Skripkin

1st patch is correct.
2nd patch is a good catch, but I'm not sure if simply ignoring is good.
If mftmirr is broken / missing, then theoretically we can continue working.
But still it's a major fs error.
I'm thinking about exiting mount with error, and if "force" is present,
then continue with mount.
I'll reply again when I'll be sure what is correct behavior.
Thank you for your work!

Re: [PATCH 1/2] ntfs3: fix NULL deref in ntfs_update_mftmirr

[Konstantin Komarov]

On 6/9/22 19:42, Konstantin Komarov wrote:
> 
> 
> On 6/4/22 14:42, Pavel Skripkin wrote:
>> On 4/21/22 23:53, Pavel Skripkin wrote:
>>> If ntfs_fill_super() wasn't called then sbi->sb will be equal to NULL.
>>> Code should check this ptr before dereferencing. Syzbot hit this issue
>>> via passing wrong mount param as can be seen from log below
>>>
>>> Fail log:
>>> ntfs3: Unknown parameter 'iochvrset'
>>> general protection fault, probably for non-canonical address 0xdffffc0000000003: 0000 [#1] PREEMPT SMP KASAN
>>> KASAN: null-ptr-deref in range [0x0000000000000018-0x000000000000001f]
>>> CPU: 1 PID: 3589 Comm: syz-executor210 Not tainted 5.18.0-rc3-syzkaller-00016-gb253435746d9 #0
>>> ...
>>> Call Trace:
>>>   <TASK>
>>>   put_ntfs+0x1ed/0x2a0 fs/ntfs3/super.c:463
>>>   ntfs_fs_free+0x6a/0xe0 fs/ntfs3/super.c:1363
>>>   put_fs_context+0x119/0x7a0 fs/fs_context.c:469
>>>   do_new_mount+0x2b4/0xad0 fs/namespace.c:3044
>>>   do_mount fs/namespace.c:3383 [inline]
>>>   __do_sys_mount fs/namespace.c:3591 [inline]
>>>
>>> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
>>> Reported-and-tested-by: syzbot+c95173762127ad76a824@syzkaller.appspotmail.com
>>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>>
>> gentle ping
>>
>>
>>
>>
>> With regards,
>> Pavel Skripkin
> 
> 1st patch is correct.
> 2nd patch is a good catch, but I'm not sure if simply ignoring is good.
> If mftmirr is broken / missing, then theoretically we can continue working.
> But still it's a major fs error.
> I'm thinking about exiting mount with error, and if "force" is present,
> then continue with mount.
> I'll reply again when I'll be sure what is correct behavior.
> Thank you for your work!

I've accepted both patches.
I don't want to invent convoluted logic for mftmirr.
Thanks again for your work!