Re: [PATCH] ocfs2: fix out-of-bounds write in ocfs2_remove_refcount_extent

[Joseph Qi]

On 6/2/26 2:44 AM, Ian Bridges wrote:
> [BUG]
> Unlinking a refcounted file whose refcount tree has leaf blocks
> triggers a fortify panic due to an out-of-bounds write.
> 
> [CAUSE]
> When the last leaf block is removed from a refcount tree,
> ocfs2_remove_refcount_extent() converts the root back to leaf mode
> with a bulk memset on &rb->rf_records. rf_records sits in an anonymous
> union with rf_list. rf_list.l_tree_depth aliases rf_records.rl_count,
> and is 0 for a single-level tree. With rl_count equal to 0, the memset
> writes past the 16-byte declared size of rf_records, which the fortify
> checker catches.
> 
> [FIX]
> Replace the bulk memset on &rb->rf_records with a correctly-bounded
> memset on rl_recs[] alone, after setting rl_count to the correct value.
> 
> Fixes: 2f26f58df041 ("ocfs2: annotate flexible array members with __counted_by_le()")
> Reported-by: syzbot+3ef989aae096b30f1663@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=3ef989aae096b30f1663
> Signed-off-by: Ian Bridges <icb@xxxxxxxxxxxx>

Looks good.
Reviewed-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>

> ---
> This patch contains a proposed fix for a crash reported by syzbot
> in __ocfs2_decrease_refcount().
> 
> The file names and offsets in this description are from commit
> 7cb1c5b32a2bfde961fff8d5204526b609bcb30a from this repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> 
> I also have a small test harness that reproduces the original panic,
> which I can make available as well.
> 
> The Bug
> 
> The ocfs2_refcount_list structure (fs/ocfs2/ocfs2_fs.h:935) contains
> two fields relevant to this bug: rl_count, the maximum number of
> refcount record slots, and rl_used, the number of occupied slots. The
> flexible array member rl_recs is annotated __counted_by_le(rl_count)
> (fs/ocfs2/ocfs2_fs.h:942), which instructs the compiler to use
> rl_count as the runtime bound for __builtin_dynamic_object_size
> queries on rl_recs when CONFIG_FORTIFY_SOURCE is enabled.
> 
> ocfs2_remove_refcount_extent() (fs/ocfs2/refcounttree.c:2066) is
> called when the last refcount record is removed from a leaf block. If
> that leaf was also the last one, the root is converted back from tree
> mode to leaf mode. The restore path (fs/ocfs2/refcounttree.c:2131–2137)
> zeros the header fields and resets the on-disk record area with a memset
> on &rb->rf_records (fs/ocfs2/refcounttree.c:2134).
> 
> This is an out-of-bounds write of the rf_records struct member.
> rf_records and rf_list share the same memory in an anonymous union; the
> first field of rf_list, l_tree_depth, occupies the same bytes as
> rf_records.rl_count. A refcount tree is always single-level, so
> l_tree_depth is 0, which means rl_count reads as 0 at the memset call
> site. The declared size of rf_records is therefore 16 bytes (the fixed
> header with zero record slots), but the memset writes the full remainder
> of the block (past the end of the struct member).
> 
> Here is a step-by-step breakdown of how the crash is triggered by
> unlinking a refcounted file:
> 
> 0. Inode eviction calls through to ocfs2_commit_truncate()
> (fs/ocfs2/alloc.c:7241), which calls ocfs2_remove_btree_range()
> (fs/ocfs2/alloc.c:5714) for each extent range. Because the extents
> are refcounted, ocfs2_decrease_refcount() (fs/ocfs2/alloc.c:5800)
> is called to decrement the reference count for each cluster.
> 1. For each cluster whose reference count reaches zero,
> ocfs2_decrease_refcount_rec() (fs/ocfs2/refcounttree.c:2158)
> removes the corresponding record from the leaf block.
> 2. When the last record is removed from a leaf block, rl_used reaches
> 0 and ocfs2_remove_refcount_extent() (fs/ocfs2/refcounttree.c:2203)
> is called to detach it from the tree root's extent list.
> 3. If that was the last leaf, the root has no remaining extents and
> the restore-to-leaf-mode path at fs/ocfs2/refcounttree.c:2125 is
> entered.
> 4. The memset at fs/ocfs2/refcounttree.c:2134 writes past the
> 16-byte declared size of rf_records. The fortify checker reads
> rl_count, which is 0 because it aliases l_tree_depth in the union,
> and fires.
> 
> The Proposed Fix
> 
> Replace the bulk memset on &rb->rf_records with explicit initialisation
> of each header field of ocfs2_refcount_list, followed by a
> correctly-bounded memset on rl_recs[] alone. rl_count is set before the
> memset so that the fortify checker can read it at runtime to verify the
> write against the declared array size.
> 
>  fs/ocfs2/refcounttree.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index 8eee5be4d1ed..7323bde70caa 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -2131,10 +2131,15 @@ static int ocfs2_remove_refcount_extent(handle_t *handle,
>  		rb->rf_flags = 0;
>  		rb->rf_parent = 0;
>  		rb->rf_cpos = 0;
> -		memset(&rb->rf_records, 0, sb->s_blocksize -
> -		       offsetof(struct ocfs2_refcount_block, rf_records));
> +		rb->rf_records.rl_used = 0;
> +		rb->rf_records.rl_reserved2 = 0;
> +		rb->rf_records.rl_reserved1 = 0;
> +		/* rl_count determines the memset size and fortify object size. */
>  		rb->rf_records.rl_count =
>  				cpu_to_le16(ocfs2_refcount_recs_per_rb(sb));
> +		memset(rb->rf_records.rl_recs, 0,
> +		       le16_to_cpu(rb->rf_records.rl_count) *
> +		       sizeof(*rb->rf_records.rl_recs));
>  	}
>  
>  	ocfs2_journal_dirty(handle, ref_root_bh);