[PATCH] ocfs2: validate la_size before ocfs2_clear_local_alloc()

[PATCH] ocfs2: validate la_size before ocfs2_clear_local_alloc()

[Dmitry Morgun]

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 <d.morgun@ispras.ru>
---
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.

 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);
-- 
2.34.1

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

[Joseph Qi]

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 <d.morgun@ispras.ru>
> ---
> 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
ocfs2-devel@lists.linux.dev 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);

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

[d.morgun]

On 8/11/26 9:57 AM, Joseph Qi wrote:
> I think we can validate localalloc inode during block read in
> ocfs2_validate_inode_block(), which will drop the duplicate code in
> callers.

Hi Joseph,

Thanks for the suggestion. I moved the la_size check there, and it
does catch the current reproducer during mount. Before dropping the
duplicate checks in ocfs2_begin_local_alloc_recovery() and
ocfs2_load_local_alloc(), I'd like to raise two concerns:

1. ocfs2_begin_local_alloc_recovery() reads the inode with
    OCFS2_BH_IGNORE_CACHE, and ocfs2_read_blocks() skips validation
    for dirty buffers. I traced through jbd2 replay: 
ocfs2_replay_journal()
    calls jbd2_journal_flush() right after jbd2_journal_load(), and
    jbd2_journal_recover() itself calls sync_blockdev(), which should
    clear the dirty flag before ocfs2_begin_local_alloc_recovery() runs.
    So this may not be reachable in practice, but I haven't been able
    to confirm it directly. The current reproducer hits the
    validator with a clean buffer during the initial mount, before
    recovery is reached at all. Is it possible to confirm that the
    buffer is guaranteed to be clean by that point?

2. The check is gated on OCFS2_LOCAL_ALLOC_FL, but
    ocfs2_validate_inode_block() doesn't receive the inode's expected
    type, so it can't verify that the on-disk flags actually match
    what the caller expects. A corrupted local alloc inode with the
    flag cleared would skip the check, even though
    ocfs2_clear_local_alloc() still treats its id2 as i_lab. Is there
    an existing way to check this correspondence? Otherwise, this
    might call for a separate per-type validation helper that takes
    the expected inode type, which could also help elsewhere.

My inclination is: v1 (the check directly in
ocfs2_begin_local_alloc_recovery() and ocfs2_load_local_alloc())
has held up in testing so far, with a small additional fix on my
end. Both call sites operate in the LOCAL_ALLOC_SYSTEM_INODE context
and check la_size before using id2 as i_lab, so they do not depend on
OCFS2_LOCAL_ALLOC_FL for deciding whether to validate la_size.
Returning this new error also made a separate, pre-existing bug in
the recovery thread reachable, for which I have a small follow-up
fix.

If the two concerns with ocfs2_validate_inode_block() can be
addressed without too much churn, I agree it is the better place
for this check, since it would cover every caller uniformly.
Otherwise, I'd rather keep it in v1's approach.

Does this seem reasonable, or would you weigh it differently?

Thanks,
Dmitry