[PATCH v2] ext4: add reserved GDT blocks check

[PATCH v2] ext4: add reserved GDT blocks check

[Zhang Yi]

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(+)

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

Re: [PATCH v2] ext4: add reserved GDT blocks check

[Jan Kara]

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] ext4: add reserved GDT blocks check

[Ritesh Harjani]

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
>

Re: [PATCH v2] ext4: add reserved GDT blocks check

[Theodore Ts'o]

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,
-- 
Theodore Ts'o <tytso@mit.edu>