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: [email protected]
Signed-off-by: Pavel Skripkin <[email protected]>
---
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
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 <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Pavel Skripkin <[email protected]>
gentle ping
With regards,
Pavel Skripkin
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: [email protected]
>> Signed-off-by: Pavel Skripkin <[email protected]>
>
> 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!
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: [email protected]
>>> Signed-off-by: Pavel Skripkin <[email protected]>
>>
>> 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!