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

[Jan Kara]

On Sat 19-09-26 19:39:40, Kaixuan Li wrote:
> 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@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> 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;
>  }
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR