linux-ext4 - Re: [PATCH] ext4: add reserved GDT blocks check

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] [day] [month] [year] [list]

Message-ID: <95609364-e148-afe2-42fa-57cc91ed15d6@huawei.com>
Date:   Mon, 30 May 2022 12:55:18 +0800
From:   Zhang Yi <yi.zhang@...wei.com>
To:     Ritesh Harjani <ritesh.list@...il.com>
CC:     <linux-ext4@...r.kernel.org>, <tytso@....edu>,
        <adilger.kernel@...ger.ca>, <jack@...e.cz>, <yukuai3@...wei.com>,
        <lilingfeng3@...wei.com>
Subject: Re: [PATCH] ext4: add reserved GDT blocks check

On 2022/5/28 23:01, Ritesh Harjani wrote:
> On 22/05/26 03:32PM, Zhang Yi wrote:
>> We capture a NULL pointer issue when resizing a corrupt ext4 image which
>> is freshly clear resize_inode feature (not run e2fsck). It could be
>> simply reproduced by following steps. The problem is because of the
>> resize_inode feature was cleared, and it will convert the filesystem to
>> meta_bg mode in ext4_resize_fs(), but the es->s_reserved_gdt_blocks was
>> not reduced to zero, so could we mistakenly call reserve_backup_gdb()
>> and passing an uninitialized resize_inode to it when adding new group
>> descriptors.
>>
>>  mkfs.ext4 /dev/sda 3G
>>  tune2fs -O ^resize_inode /dev/sda #forget to run requested e2fsck
>>  mount /dev/sda /mnt
>>  resize2fs /dev/sda 8G
>>
>>  ========
>>  BUG: kernel NULL pointer dereference, address: 0000000000000028
>>  CPU: 19 PID: 3243 Comm: resize2fs Not tainted 5.18.0-rc7-00001-gfde086c5ebfd #748
>>  ...
>>  RIP: 0010:ext4_flex_group_add+0xe08/0x2570
>>  ...
>>  Call Trace:
>>   <TASK>
>>   ext4_resize_fs+0xbec/0x1660
>>   __ext4_ioctl+0x1749/0x24e0
>>   ext4_ioctl+0x12/0x20
>>   __x64_sys_ioctl+0xa6/0x110
>>   do_syscall_64+0x3b/0x90
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>>  RIP: 0033:0x7f2dd739617b
>>  ========
>>
>> The fix is simple, add a check in ext4_resize_fs() to make sure that the
>> es->s_reserved_gdt_blocks is zero when the resize_inode feature is
>> disabled.
> 
> Your reasoning looks correct to me.
> 
>>
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
>> ---
>>  fs/ext4/resize.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
>> index 90a941d20dff..5791eb7c0761 100644
>> --- a/fs/ext4/resize.c
>> +++ b/fs/ext4/resize.c
>> @@ -2031,6 +2031,9 @@ int ext4_resize_fs(struct super_block *sb, ext4_fsblk_t n_blocks_count)
>>  			ext4_warning(sb, "Error opening resize inode");
>>  			return PTR_ERR(resize_inode);
>>  		}
>> +	} else if (es->s_reserved_gdt_blocks) {
>> +		ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero");
>> +		return -EFSCORRUPTED;
>>  	}
> 
> I think we should do this check in ext4_resize_begin(), i.e.
> if ext4_has_feature_resize_inode() is false and es->s_reserved_gdt_blocks is
> non-zero, then we should straight away mark and return error.
> 

Thanks for your suggestion. Yes, put this check in ext4_resize_begin() looks
better, it is also useful for EXT4_IOC_GROUP_ADD (although we have a check in
ext4_group_add() already, it is still worth). I will put this check into
ext4_resize_begin() in my next iteration.

> Later (not as part of this patch/fix) maybe if we detect this problem, we could
> use helpers like ext4_update_super() to fix this mismatch problem in kernel
> during mount itself. But I think this is not absolutely necessary,
> as kernel already during mount outputs a warning and ask for running e2fsck.
> 

I think the warning from mount outputs is enough, sysadmins should run e2fsck
after getting this note. It's hard and costly if we want to fix this inconsistent
problem in kernel because from the kernel's point of view, if it detect above
inconsistency, it means that both of the es->s_reserved_gdt_blocks and resize_inode
feature are not trusted anymore, we have to do in depth check as e2fsck does, and
it's hard to make a fix decision automatically(not even e2fsck).

Thanks,
Yi.