Messages in this thread Patch in this message | | | Subject | Re: [PATCH 1/2] quota: fix null-ptr-deref in ext4_acquire_dquot() | From | "yebin (H)" <> | Date | Sat, 3 Jun 2023 14:14:11 +0800 |
| |
On 2023/5/30 17:57, Jan Kara wrote: > On Sat 27-05-23 09:40:17, Ye Bin wrote: >> Syzbot found the following issue: >> Unable to handle kernel paging request at virtual address dfff800000000005 >> KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f] > ... >> CPU: 0 PID: 6080 Comm: syz-executor747 Not tainted 6.3.0-rc7-syzkaller-g14f8db1c0f9a #0 >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/14/2023 >> pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) >> pc : ext4_acquire_dquot+0x1d4/0x398 fs/ext4/super.c:6766 >> lr : dquot_to_inode fs/ext4/super.c:6740 [inline] >> lr : ext4_acquire_dquot+0x1ac/0x398 fs/ext4/super.c:6766 > OK, this is bad... > >> Above issue may happens as follows: >> ProcessA ProcessB ProcessC >> sys_fsconfig >> vfs_fsconfig_locked >> reconfigure_super >> ext4_remount >> dquot_suspend -> suspend all type quota >> >> sys_fsconfig >> vfs_fsconfig_locked >> reconfigure_super >> ext4_remount >> dquot_resume >> ret = dquot_load_quota_sb >> add_dquot_ref >> do_open -> open file O_RDWR >> vfs_open >> do_dentry_open >> get_write_access >> atomic_inc_unless_negative(&inode->i_writecount) >> ext4_file_open >> dquot_file_open >> dquot_initialize >> __dquot_initialize >> dqget >> if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags)) >> >> __dquot_initialize >> __dquot_initialize >> dqget >> if (!test_bit(DQ_ACTIVE_B, &dquot->dq_flags)) >> ext4_acquire_dquot -> Return error >> if (ret < 0) >> vfs_cleanup_quota_inode >> dqopt->files[type] = NULL; > But I don't see how this can happen. The code in dquot_load_quota_sb() > looks like: > > error = add_dquot_ref(sb, type); > if (error) > dquot_disable(sb, type, flags); > > So if an error happens in add_dquot_ref(), we'll call dquot_disable(). > dquot_disable() then does: > > drop_dquot_ref(sb, cnt); > invalidate_dquots(sb, cnt); > > and invalidate_dquots() waits for reference count of all dquots to drop to > 0. Hence if dqget() returned a dquot pointer to ProcessC, then ProcessB > should wait until ProcessC drops the dquot reference (hence > ext4_acquire_dquot() is done). > > What am I missing? > > Honza My reproduction condition is: mkfs.ext4 -F /dev/sda tune2fs -Q usrquota /dev/sda
dquot_disable ... if ((flags & DQUOT_USAGE_ENABLED && !(flags & DQUOT_LIMITS_ENABLED)) || (flags & DQUOT_SUSPENDED && flags & (DQUOT_LIMITS_ENABLED | DQUOT_USAGE_ENABLED))) return -EINVAL; ... If without enable DQUOT_LIMITS_ENABLED dquot_disable() will just return -EINVAL. >> ext4_acquire_dquot >> -->dquot_to_inode(dquot) is NULL >> >> To solve above issue, if quota has been loaded, there's unneed to cleaup quota >> inode if dquot_load_quota_sb() return failed when do dquot_resume(); >> >> Reported-by: syzbot+e633c79ceaecbf479854@syzkaller.appspotmail.com >> Signed-off-by: Ye Bin <yebin10@huawei.com> >> --- >> fs/quota/dquot.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c >> index ffd40dc3e4e9..4b913faa48ec 100644 >> --- a/fs/quota/dquot.c >> +++ b/fs/quota/dquot.c >> @@ -2476,7 +2476,7 @@ int dquot_resume(struct super_block *sb, int type) >> flags = dquot_generic_flag(flags, cnt); >> ret = dquot_load_quota_sb(sb, cnt, dqopt->info[cnt].dqi_fmt_id, >> flags); >> - if (ret < 0) >> + if (ret < 0 && !sb_has_quota_loaded(sb, cnt)) >> vfs_cleanup_quota_inode(sb, cnt); >> } >> >> -- >> 2.31.1 >>
| |