Re: [PATCH] ocfs2: validate la_size before ocfs2_clear_local_alloc()

On 8/10/26 7:35 PM, Dmitry Morgun wrote:
> la_size, like the dirty flag, is read from disk. If dirty != 0,
> ocfs2_begin_local_alloc_recovery() is always called, which
> immediately invokes ocfs2_clear_local_alloc(). At this point,
> ocfs2_clear_local_alloc() uses la_size as the loop bound without
> validating it first. If la_size is corrupted, the loop writes past
> the end of la_bitmap and may eventually start writing into memory
> that has already been freed.
> 
> BUG: KASAN: use-after-free in ocfs2_clear_local_alloc fs/ocfs2/localalloc.c:919 [inline]
> BUG: KASAN: use-after-free in ocfs2_begin_local_alloc_recovery+0xb07/0xc00 fs/ocfs2/localalloc.c:515
> CPU: 1 PID: 2386 Comm: syz.2.190 Not tainted 6.1.174-syzkaller-00520-g10c505401422 #0
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-debian-1.17.0-1 04/01/2014
> Call Trace:
>  <TASK>
>  ocfs2_clear_local_alloc fs/ocfs2/localalloc.c:919 [inline]
>  ocfs2_begin_local_alloc_recovery+0xb07/0xc00 fs/ocfs2/localalloc.c:515
>  ocfs2_check_volume fs/ocfs2/super.c:2448 [inline]
>  ocfs2_mount_volume fs/ocfs2/super.c:1819 [inline]
>  ocfs2_fill_super+0x2033/0x3dc0 fs/ocfs2/super.c:1082
>  mount_bdev+0x356/0x410 fs/super.c:1443
>  legacy_get_tree+0x108/0x220 fs/fs_context.c:632
>  vfs_get_tree+0x8e/0x300 fs/super.c:1573
>  do_new_mount fs/namespace.c:3078 [inline]
>  path_mount+0x6af/0x1f70 fs/namespace.c:3408
>  do_mount fs/namespace.c:3421 [inline]
>  __do_sys_mount fs/namespace.c:3629 [inline]
>  __se_sys_mount fs/namespace.c:3606 [inline]
>  __x64_sys_mount+0x283/0x300 fs/namespace.c:3606
>  do_syscall_x64 arch/x86/entry/common.c:46 [inline]
>  do_syscall_64+0x35/0x80 fs/namespace.c:76
>  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> 
> The validation of la_size currently exists only in
> ocfs2_load_local_alloc(), but that function is called after
> ocfs2_clear_local_alloc(). As a result, memory corruption occurs
> before the invalid value is detected and -EINVAL is returned.
> 
> Adding the same validation to ocfs2_begin_local_alloc_recovery()
> before calling ocfs2_clear_local_alloc() prevents the out-of-bounds
> write by failing the recovery early with -EINVAL.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: ccd979bdbce9 ("OCFS2: The Second Oracle Cluster Filesystem")
> Signed-off-by: Dmitry Morgun <[email protected]>
> ---
> A similar issue also exists in ocfs2_complete_local_alloc_recovery().
> The i_total and la_bm_off fields are also read from disk and used as
> loop bounds and offsets without prior validation. With a corrupted
> filesystem image, they could lead to similar out-of-bounds accesses
> during the completion of local alloc recovery. Therefore, a more
> complete solution would be to introduce a shared validation helper
> for all relevant on-disk local alloc fields (la_size, i_total,
> la_bm_off, and others) and invoke it before starting the recovery
> process.
> 
> This patch fixes only the reported reproducer, triggered 
> by an invalid la_size. Other fields like i_total and la_bm_off are
> still unvalidated, so a similarly corrupted image could trigger 
> an analogous bug elsewhere. Maintainers' input would be welcome 
> on whether a shared validation helper is preferred over targeted 
> per-field fixes.
> 

I think we can validate localalloc inode during block read in
ocfs2_validate_inode_block(), which will drop the duplicate code in
callers.

BTW, it seems you post it into a wrong maillist. Please use
[email protected] instead.

Thanks,
Joseph

>  fs/ocfs2/localalloc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fs/ocfs2/localalloc.c b/fs/ocfs2/localalloc.c
> index c4426d12a..ce03903ed 100644
> --- a/fs/ocfs2/localalloc.c
> +++ b/fs/ocfs2/localalloc.c
> @@ -481,6 +481,7 @@ int ocfs2_begin_local_alloc_recovery(struct ocfs2_super *osb,
>  	struct buffer_head *alloc_bh = NULL;
>  	struct inode *inode = NULL;
>  	struct ocfs2_dinode *alloc;
> +	struct ocfs2_local_alloc *la;
>  
>  	trace_ocfs2_begin_local_alloc_recovery(slot_num);
>  
> @@ -512,6 +513,16 @@ int ocfs2_begin_local_alloc_recovery(struct ocfs2_super *osb,
>  	memcpy((*alloc_copy), alloc_bh->b_data, alloc_bh->b_size);
>  
>  	alloc = (struct ocfs2_dinode *) alloc_bh->b_data;
> +	la = OCFS2_LOCAL_ALLOC(alloc);
> +
> +	if ((la->la_size == 0) ||
> +		(le16_to_cpu(la->la_size) > ocfs2_local_alloc_size(inode->i_sb))) {
> +		mlog(ML_ERROR, "Local alloc size is invalid (la_size = %u)\n",
> +			le16_to_cpu(la->la_size));
> +		status = -EINVAL;
> +		goto bail;
> +	}
> +
>  	ocfs2_clear_local_alloc(alloc);
>  
>  	ocfs2_compute_meta_ecc(osb->sb, alloc_bh->b_data, &alloc->i_check);