| Message ID | 20250329111654.5764-1-qasdev00@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] ocfs2: Validate chain list bits per cluster to prevent div-by-zero | expand |
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);
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)) { 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; + } + status = ocfs2_reserve_clusters_with_limit(osb, le16_to_cpu(cl->cl_cpg), max_block, flags, &ac);
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 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: 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(-)