ocfs2: Validate chain list bits per cluster to prevent div-by-zero
Subsystems (2)
PATCH RESEND 1/1 ocfs2: Validate chain list bits per cluster to prevent div-by-zero
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;
}--- fs/ocfs2/resize.c | 4 ++-- fs/ocfs2/suballoc.c | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-)
fs/ocfs2/resize.c +2 -2
@@ -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; }
fs/ocfs2/suballoc.c +5 -0
@@ -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);
Signature · 1 line
2.39.5
Heming Zhao
reviewerHi,
Quoted 11 lines · Qasim Ijaz
The call trace shows that the div error occurs on the following line where the code sets the
e_cposmember of the extent record while dividingbg_bitsby 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:
Quoted 7 lines
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 inocfs2_block_group_alloc(), cl is derived fromocfs2_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.
Quoted 4 lines
verify the
cl_bpcmember 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_bpcmembers 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.
Quoted 24 lines
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 <[email protected]>Signed-off-by Qasim Ijaz <[email protected]>--- fs/ocfs2/resize.c | 4 ++-- fs/ocfs2/suballoc.c | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-)fs/ocfs2/resize.c +0
-0
fs/ocfs2/resize.c int ocfs2_group_extend(struct inode * inode, int new_clusters) · line 333
Quoted 7 lines
fs/ocfs2/resize.c +2
-2@@ -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.
Quoted 8 lines
fs/ocfs2/resize.c +0
-0ret = -EINVAL ; goto out_unlock; }fs/ocfs2/suballoc.c +0
-0
fs/ocfs2/suballoc.c static int ocfs2_block_group_alloc(struct ocfs2_super *osb, · line 678
Quoted 8 lines
fs/ocfs2/suballoc.c +5
-0@@ -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
Quoted 3 lines
fs/ocfs2/suballoc.c +0
-0status =ocfs2_reserve_clusters_with_limit (osb,le16_to_cpu (cl->cl_cpg), max_block, flags, &ac);