Re: [PATCH] ext4: reject impossible group free cluster counts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: journal replay bypasses ext4_check_replayed_free_clusters()
--

commit 57978228cd9b85af900a61d55a732ad888e33b8c
Author: Zhao Gongyi <zhaogongyi@bytedance.com>

ext4: reject impossible group free cluster counts

This commit rejects group descriptor free cluster counts that exceed the
group's cluster capacity during mount-time descriptor validation, and
rechecks the free cluster counts after journal replay when recovery is
needed.

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 7d8aedfbbb87f..b7dd1d4a3b179 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3355,6 +3355,36 @@ void ext4_group_desc_csum_set(struct super_block *sb, __u32 block_group,
>  	gdp->bg_checksum = ext4_group_desc_csum(sb, block_group, gdp);
>  }
>  
> +static bool ext4_check_group_free_clusters(struct super_block *sb,
> +					   ext4_group_t group,
> +					  struct ext4_group_desc *gdp)
> +{
> +	unsigned int free_clusters = ext4_free_group_clusters(sb, gdp);
> +	unsigned int group_clusters = ext4_num_clusters_in_group(sb, group);
> +
> +	if (free_clusters > group_clusters) {
> +		ext4_msg(sb, KERN_ERR,
> +			 "%s: group %u has invalid free clusters count %u > %u",
> +			 __func__, group, free_clusters, group_clusters);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static int ext4_check_replayed_free_clusters(struct super_block *sb)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	ext4_group_t group;
> +
> +	for (group = 0; group < sbi->s_groups_count; group++) {
> +		struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, NULL);
> +
> +		if (!ext4_check_group_free_clusters(sb, group, gdp))
> +			return -EFSCORRUPTED;
> +	}
> +	return 0;
> +}

[ ... ]

> @@ -5567,6 +5599,12 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  		err = ext4_load_and_init_journal(sb, es, ctx);
>  		if (err)
>  			goto failed_mount3a;
> +		/* Journal replay may change descriptors after the initial check. */
> +		if (needs_recovery) {
> +			err = ext4_check_replayed_free_clusters(sb);
> +			if (err)
> +				goto failed_mount_wq;
> +		}

[Severity: High]
Can a journal transaction that replays both the superblock buffer and a
group descriptor buffer bypass ext4_check_replayed_free_clusters()?

During __ext4_fill_super(), ext4_check_geometry() and
ext4_group_desc_init() -> ext4_check_descriptors() validate the initial
superblock and group descriptors before journal recovery. Next,
ext4_load_and_init_journal() -> ext4_load_journal() ->
jbd2_journal_load() -> jbd2_journal_recover() -> jbd2_do_replay() replays
journal blocks into the block device buffer cache.

Because sbi->s_es points directly into sbi->s_sbh->b_data, a journal
transaction containing logical_sb_block can overwrite s_blocks_count_lo,
s_blocks_count_hi, and s_first_data_block in sbi->s_es (ext4_load_journal()
only saves and restores the EXT4_S_ERR_START..EXT4_S_ERR_END range).

When ext4_check_replayed_free_clusters() subsequently calls
ext4_check_group_free_clusters() -> ext4_num_clusters_in_group() for the
final block group:

fs/ext4/balloc.c:ext4_num_clusters_in_group() {
	if (block_group == ext4_get_groups_count(sb) - 1) {
		blocks = ext4_blocks_count(EXT4_SB(sb)->s_es) -
			ext4_group_first_block_no(sb, block_group);
	} else
		blocks = EXT4_BLOCKS_PER_GROUP(sb);
	return EXT4_NUM_B2C(EXT4_SB(sb), blocks);
}

blocks is computed from the live, unvalidated sbi->s_es buffer. If the
replayed s_blocks_count is smaller than ext4_group_first_block_no(sb,
block_group), the unsigned int blocks calculation underflows to near
UINT_MAX; if larger, blocks can exceed EXT4_BLOCKS_PER_GROUP(sb).
EXT4_NUM_B2C() then returns an inflated group_clusters upper bound, allowing
an impossible replayed bg_free_blocks_count to pass the free_clusters <=
group_clusters check and corrupt in-memory free-cluster accounting as well
as bitmap initialization in ext4_init_block_bitmap() and
ext4_valid_block_bitmap_padding().

In addition, only calling ext4_check_replayed_free_clusters() after
recovery leaves any replayed changes to bg_block_bitmap, bg_inode_bitmap,
bg_inode_table, bg_checksum, and bg_flags (EXT4_BG_INODE_ZEROED for
first_not_zeroed) unvalidated. Should superblock geometry and full group
descriptors via ext4_check_descriptors() be revalidated after journal
replay instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102042428.65204-1-zhaogongyi@bytedance.com?part=1