[PATCH] ext4: stop verify_reserved_gdb() one entry short of the block

[PATCH] ext4: stop verify_reserved_gdb() one entry short of the block

[Kaixuan Li]

verify_reserved_gdb() walks the backup group list, reading one __le32 from
the reserved GDT block per iteration, and bounds the walk after the read:

	if (le32_to_cpu(*p++) !=
	    grp * EXT4_BLOCKS_PER_GROUP(sb) + blk){
		...
		return -EINVAL;
	}
	if (++gdbackups > EXT4_ADDR_PER_BLOCK(sb))
		return -EFBIG;

The block holds exactly EXT4_ADDR_PER_BLOCK(sb) entries, so when gdbackups
reaches that value the test is false, the loop runs once more, and *p++
reads one entry past the block before -EFBIG is returned. The same value is
also the largest gdbackups the function can return, and
reserve_backup_gdb() then uses it as an index into a block of exactly that
many entries:

	data = (__le32 *)primary[i]->b_data;
	data[gdbackups] = cpu_to_le32(blk + primary[i]->b_blocknr);

which writes one entry past. Comparing with >= stops the walk before the
read and caps the return value one lower, which closes both.

The walk enumerates the groups ext4_list_backups() produces -- the powers
of 3, 5 and 7 below the group being added -- so EXT4_ADDR_PER_BLOCK of
them, 256 at a 1K block size and 1024 at 4K, cannot occur within a 32-bit
group number. The entry the old bound went on to read was therefore never a
real backup.

Signed-off-by: Kaixuan Li <kaixuanli0131@gmail.com>
---
Reproduced on v7.2.4 x86_64 with KASAN enabled, and on v6.12.9 before that.
Three images, one reproducer, the resize target passed on the kernel
command line:

	case                     stock                  patched
	A plain, 8 -> 32 groups  0, 65537 -> 262145     0, 65537 -> 262145
	B group 257 (APB + 1)    0, 2105345 -> 2113537  -EFBIG, no growth
	C ^sparse_super walk     -EINVAL                -EFBIG

	(APB is EXT4_ADDR_PER_BLOCK, 256 at the 1K block size used here.)

B is the write, and the thing to notice is that the resize SUCCEEDS on the
stock kernel -- this is not an operation that aborts after the access:

	BUG: KASAN: slab-use-after-free in ext4_flex_group_add+0x50f0/0x5680

C is the read:

	BUG: KASAN: slab-use-after-free in verify_reserved_gdb.isra.0+0x270/0x290

A is the control that matters for a one-character change: an ordinary
online resize goes through reserve_backup_gdb() -> verify_reserved_gdb()
and is unchanged by the patch, same return value and same resulting block
count.

Building B needs the group being added to be exactly EXT4_ADDR_PER_BLOCK+1:
below that the index is in bounds, above it the read defect trips -EFBIG
first and the call aborts before the write. At a 1K block size that is
group 257, so the image is built with groups 0..256 already present and the
resize adds only 257. sparse_super is cleared with debugfs so the walk runs
end-1 times rather than skipping most groups.

Mounting a crafted image is not a Linux kernel vulnerability
(Documentation/process/threat-model.rst), and I am not reporting it as one.
I have not attached a Fixes: tag: the bound reads the same in v2.6.32, so
it predates the git history I can bisect over.
---
 fs/ext4/resize.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -794,11 +794,11 @@ static int verify_reserved_gdb(struct super_block *sb,
 				     grp *
 				     (ext4_fsblk_t)EXT4_BLOCKS_PER_GROUP(sb) +
 				     blk);
 			return -EINVAL;
 		}
-		if (++gdbackups > EXT4_ADDR_PER_BLOCK(sb))
+		if (++gdbackups >= EXT4_ADDR_PER_BLOCK(sb))
 			return -EFBIG;
 	}
 
 	return gdbackups;
 }

Re: [PATCH] ext4: stop verify_reserved_gdb() one entry short of the block

[sashiko-bot]

> verify_reserved_gdb() walks the backup group list, reading one __le32 from
> the reserved GDT block per iteration, and bounds the walk after the read:
> 
> 	if (le32_to_cpu(*p++) !=
> 	    grp * EXT4_BLOCKS_PER_GROUP(sb) + blk){
> 		...
> 		return -EINVAL;
> 	}
> 	if (++gdbackups > EXT4_ADDR_PER_BLOCK(sb))
> 		return -EFBIG;
> 
> The block holds exactly EXT4_ADDR_PER_BLOCK(sb) entries, so when gdbackups
> reaches that value the test is false, the loop runs once more, and *p++
> reads one entry past the block before -EFBIG is returned. The same value is
> also the largest gdbackups the function can return, and
> [ ... ]
> them, 256 at a 1K block size and 1024 at 4K, cannot occur within a 32-bit
> group number. The entry the old bound went on to read was therefore never a
> real backup.
> 
> Signed-off-by: Kaixuan Li <kaixuanli0131@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919113940.1823559-1-kaixuanli0131@gmail.com?part=1