Re: [PATCH] ocfs2: validate cl_bpc in ocfs2_block_group_alloc to prevent divide-by-zero

[Joseph Qi]

On 2025/10/26 21:26, Deepanshu Kartikey wrote:
> The chain allocator field cl_bpc (blocks per cluster) is read from disk
> and used in division operations without validation. A corrupted filesystem
> image with cl_bpc=0 causes a divide-by-zero crash in the kernel:
> 
>   divide error: 0000 [#1] PREEMPT SMP KASAN
>   RIP: 0010:ocfs2_bg_discontig_add_extent fs/ocfs2/suballoc.c:335 [inline]
>   RIP: 0010:ocfs2_block_group_fill+0x5bd/0xa70 fs/ocfs2/suballoc.c:386
>   Call Trace:
>    ocfs2_block_group_alloc+0x7e9/0x1330 fs/ocfs2/suballoc.c:703
>    ocfs2_reserve_suballoc_bits+0x20a6/0x4640 fs/ocfs2/suballoc.c:834
>    ocfs2_reserve_new_inode+0x4f4/0xcc0 fs/ocfs2/suballoc.c:1074
>    ocfs2_mknod+0x83c/0x2050 fs/ocfs2/namei.c:306
> 
> This patch adds validation in ocfs2_block_group_alloc() to ensure cl_bpc
> matches the expected value calculated from the superblock's cluster size
> and block size. This validation follows the same pattern used elsewhere
> in OCFS2 to verify on-disk structures against known-good values derived
> from the superblock parameters.
> 
> The check is performed early in the allocation path, before any resources
> are allocated or transactions started, ensuring clean error propagation.
> If validation fails, the filesystem is marked read-only and the operation
> returns -EUCLEAN (Structure needs cleaning), prompting the administrator
> to run fsck.ocfs2.
> 
> The validation catches both:
> - Zero values that cause divide-by-zero crashes
> - Non-zero but incorrect values indicating filesystem corruption or
>   mismatched filesystem geometry
> 
> With this fix, mounting a corrupted filesystem produces:
>   OCFS2: ERROR (device loop0): ocfs2_block_group_alloc: Chain allocator
>          74 has corrupted cl_bpc: ondisk=0 expected=16
>   OCFS2: File system is now read-only.
> 
> Instead of a kernel crash.
> 
> Reported-by: syzbot+fd8af97c7227fe605d95@xxxxxxxxxxxxxxxxxxxxxxxxx
> Tested-by: syzbot+fd8af97c7227fe605d95@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=fd8af97c7227fe605d95
> Tested-by: syzbot+fd8af97c7227fe605d95@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> ---
>  fs/ocfs2/suballoc.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index 6ac4dcd54588..9f3db59890c3 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -667,10 +667,22 @@ static int ocfs2_block_group_alloc(struct ocfs2_super *osb,
>  	u16 alloc_rec;
>  	struct buffer_head *bg_bh = NULL;
>  	struct ocfs2_group_desc *bg;
> +	u16 cl_bpc, expected_bpc;
>  
>  	BUG_ON(ocfs2_is_cluster_bitmap(alloc_inode));
>  
>  	cl = &fe->id2.i_chain;
> +	cl_bpc = le16_to_cpu(cl->cl_bpc);
> +	expected_bpc = 1 << (osb->s_clustersize_bits - alloc_inode->i_sb->s_blocksize_bits);

'cl_bpc' is 'bits per cluster'.

> +	if (cl_bpc != expected_bpc) {
> +		ocfs2_error(alloc_inode->i_sb,
> +			"Chain allocator %llu has corrupted cl_bpc: ondisk=%u expected=%u\n",
> +			(unsigned long long)le64_to_cpu(fe->i_blkno),
> +			cl_bpc, expected_bpc);
> +		status = -EUCLEAN;
> +		goto bail;
> +	}
> +

Why not check this in ocfs2_validate_inode_block()? This may benifit all paths.

Thanks,
Joseph

>  	status = ocfs2_reserve_clusters_with_limit(osb,
>  						   le16_to_cpu(cl->cl_cpg),
>  						   max_block, flags, &ac);