Re: [PATCH RESEND] ocfs2: Validate chain list bits per cluster to prevent div-by-zero

[Heming Zhao <heming.zhao@suse.com>]

Hi,

On 3/29/25 19:16, Qasim Ijaz wrote:
> The call trace shows that the div error occurs on the following line where the code sets
> the e_cpos member of the extent record while dividing bg_bits by the bits per
> cluster value from the chain list:
> 
> 		rec->e_cpos = cpu_to_le32(le16_to_cpu(bg->bg_bits) /
> 				  le16_to_cpu(cl->cl_bpc));
> 				
> Looking at the code disassembly we see the problem occurred during the divw instruction
> which performs a 16-bit unsigned divide operation. The main ways a divide error can occur is
> if:
> 
> 1) the divisor is 0
> 2) if the quotient is too large for the designated register (overflow).
> 
> Normally the divisor being 0 is the most common cause for a division error to occur.
> 
> Focusing on the bits per cluster cl->cl_bpc (since it is the divisor) we see that cl is created in
> ocfs2_block_group_alloc(), cl is derived from ocfs2_dinode->id2.i_chain. To fix this issue we should

The cl (chain list) is created by ocfs2-tools during the execution of mkfs.ocfs2.
ocfs2_block_group_alloc(), as its name, which creates block groups, not chain lists.

> verify the cl_bpc member in the chain list to ensure it is valid and non-zero.
> 
> Looking through the rest of the OCFS2 code it seems like there are other places which could benefit
> from improved checks of the cl_bpc members of chain lists like the following:

Syzbot performs fuzz testing, in real-world it's very difficult to clear
the cl_bpc value. For this issue, checking only the divisor value is sufficient.

> 
> In ocfs2_group_extend():
> 
> 	cl_bpc = le16_to_cpu(fe->id2.i_chain.cl_bpc);
> 	if (le16_to_cpu(group->bg_bits) / cl_bpc + new_clusters >
> 		le16_to_cpu(fe->id2.i_chain.cl_cpg)) {
> 		ret = -EINVAL;
> 		goto out_unlock;
> 	}
> 
> Reported-by: syzbot <syzbot+e41e83af7a07a4df8051@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=e41e83af7a07a4df8051
> Cc: stable@vger.kernel.org
> Signed-off-by: Qasim Ijaz <qasdev00@gmail.com>
> ---
>   fs/ocfs2/resize.c   | 4 ++--
>   fs/ocfs2/suballoc.c | 5 +++++
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/resize.c b/fs/ocfs2/resize.c
> index b0733c08ed13..22352c027ecd 100644
> --- a/fs/ocfs2/resize.c
> +++ b/fs/ocfs2/resize.c
> @@ -329,8 +329,8 @@ int ocfs2_group_extend(struct inode * inode, int new_clusters)
>   	group = (struct ocfs2_group_desc *)group_bh->b_data;
>   
>   	cl_bpc = le16_to_cpu(fe->id2.i_chain.cl_bpc);
> -	if (le16_to_cpu(group->bg_bits) / cl_bpc + new_clusters >
> -		le16_to_cpu(fe->id2.i_chain.cl_cpg)) {
> +	if (!cl_bpc || le16_to_cpu(group->bg_bits) / cl_bpc + new_clusters >
> +		       le16_to_cpu(fe->id2.i_chain.cl_cpg)) {

checking cl_bpc makes sense.

>   		ret = -EINVAL;
>   		goto out_unlock;
>   	}
> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
> index f7b483f0de2a..844cb36bd7ab 100644
> --- a/fs/ocfs2/suballoc.c
> +++ b/fs/ocfs2/suballoc.c
> @@ -671,6 +671,11 @@ static int ocfs2_block_group_alloc(struct ocfs2_super *osb,
>   	BUG_ON(ocfs2_is_cluster_bitmap(alloc_inode));
>   
>   	cl = &fe->id2.i_chain;
> +	if (!le16_to_cpu(cl->cl_bpc)) {
> +		status = -EINVAL;
> +		goto bail;
> +	}
> +

See my previous comment, this part code is useless, please remove it.

Thanks,
Heming

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