| Message ID | 20220601092717.763694-1-yi.zhang@huawei.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [v2] ext4: add reserved GDT blocks check | expand |
On Wed 01-06-22 17:27:17, 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_begin() to make sure that > the es->s_reserved_gdt_blocks is zero when the resize_inode feature is > disabled. > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> Looks good to me. Thanks for the fix. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > v2->v1: > - move check from ext4_resize_fs() to ext4_resize_begin(). > > fs/ext4/resize.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c > index 90a941d20dff..8b70a4701293 100644 > --- a/fs/ext4/resize.c > +++ b/fs/ext4/resize.c > @@ -53,6 +53,16 @@ int ext4_resize_begin(struct super_block *sb) > if (!capable(CAP_SYS_RESOURCE)) > return -EPERM; > > + /* > + * If the reserved GDT blocks is non-zero, the resize_inode feature > + * should always be set. > + */ > + if (EXT4_SB(sb)->s_es->s_reserved_gdt_blocks && > + !ext4_has_feature_resize_inode(sb)) { > + ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero"); > + return -EFSCORRUPTED; > + } > + > /* > * If we are not using the primary superblock/GDT copy don't resize, > * because the user tools have no way of handling this. Probably a > -- > 2.31.1 >
On 22/06/01 05:27PM, 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_begin() to make sure that > the es->s_reserved_gdt_blocks is zero when the resize_inode feature is > disabled. Sure, I have verified this change at my end too with your execerciser. And having this check this in ext4_resize_begin(), looks good to me. Feel free to add - Reviewed-by: Ritesh Harjani <ritesh.list@gmail.com> > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > --- > v2->v1: > - move check from ext4_resize_fs() to ext4_resize_begin(). > > fs/ext4/resize.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c > index 90a941d20dff..8b70a4701293 100644 > --- a/fs/ext4/resize.c > +++ b/fs/ext4/resize.c > @@ -53,6 +53,16 @@ int ext4_resize_begin(struct super_block *sb) > if (!capable(CAP_SYS_RESOURCE)) > return -EPERM; > > + /* > + * If the reserved GDT blocks is non-zero, the resize_inode feature > + * should always be set. > + */ > + if (EXT4_SB(sb)->s_es->s_reserved_gdt_blocks && > + !ext4_has_feature_resize_inode(sb)) { > + ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero"); > + return -EFSCORRUPTED; > + } > + > /* > * If we are not using the primary superblock/GDT copy don't resize, > * because the user tools have no way of handling this. Probably a > -- > 2.31.1 >
On Wed, 1 Jun 2022 17:27:17 +0800, 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. > > [...] Applied, thanks! [1/1] ext4: add reserved GDT blocks check commit: 7dc0ff3a33ea92cefaf032a6d0de9314a9a5fb20 Best regards,
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c index 90a941d20dff..8b70a4701293 100644 --- a/fs/ext4/resize.c +++ b/fs/ext4/resize.c @@ -53,6 +53,16 @@ int ext4_resize_begin(struct super_block *sb) if (!capable(CAP_SYS_RESOURCE)) return -EPERM; + /* + * If the reserved GDT blocks is non-zero, the resize_inode feature + * should always be set. + */ + if (EXT4_SB(sb)->s_es->s_reserved_gdt_blocks && + !ext4_has_feature_resize_inode(sb)) { + ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero"); + return -EFSCORRUPTED; + } + /* * If we are not using the primary superblock/GDT copy don't resize, * because the user tools have no way of handling this. Probably a
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_begin() to make sure that the es->s_reserved_gdt_blocks is zero when the resize_inode feature is disabled. Signed-off-by: Zhang Yi <yi.zhang@huawei.com> --- v2->v1: - move check from ext4_resize_fs() to ext4_resize_begin(). fs/ext4/resize.c | 10 ++++++++++ 1 file changed, 10 insertions(+)