linux-kernel - Re: [PATCH v2] ocfs2: Fix NULL pointer dereference in ocfs2_get_refcount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <xdoixnwdnplg4zo7zyzkuzaqakc66rsnyqjap4axeevcc3tf7e@rgqnmdhmqsqi>
Date: Tue, 20 Jan 2026 09:46:24 +0800
From: Heming Zhao <heming.zhao@...e.com>
To: Jiasheng Jiang <jiashengjiangcool@...il.com>
Cc: jlbec@...lplan.org, joseph.qi@...ux.alibaba.com, 
	linux-kernel@...r.kernel.org, mark@...heh.com, ocfs2-devel@...ts.linux.dev
Subject: Re: [PATCH v2] ocfs2: Fix NULL pointer dereference in
 ocfs2_get_refcount_rec()

On Mon, Jan 19, 2026 at 05:12:15PM +0000, Jiasheng Jiang wrote:
> On Mon, 19 Jan 2026 22:43:26 +0800, Heming Zhao wrote:
> > On Sun, Jan 18, 2026 at 07:05:23PM +0000, Jiasheng Jiang wrote:
> >> In ocfs2_get_refcount_rec(), the 'rec' pointer is initialized to NULL.
> >> If the extent list is empty (el->l_next_free_rec == 0), the loop skips
> >> assignment, leaving 'rec' as NULL and 'found' as 0.
> >> 
> >> Currently, the code skips the 'if (found)' block but proceeds directly to
> >> dereference 'rec' at line 767 (le64_to_cpu(rec->e_blkno)), causing a
> >> NULL pointer dereference panic.
> > 
> > Do you have reproduction steps to support your analysis?
> > 
> 
> Thanks for your review. We found this issue through static analysis and
> code review. We do not have a specific reproduction script or a corrupted
> filesystem image to trigger this at the moment.
> 
> > there are two types of 'rb': leaf or tree.
> > the check 'if (!(le32_to_cpu(rb->rf_flags) & OCFS2_REFCOUNT_TREE_FL))'
> > handles leaf case and returns to the caller.
> > the subsequent code handles the 'tree' type, therefore, in theory,
> > el->l_next_free_rec should be ">= 1", and the execution should enter the
> > for-loop to assign the 'rec'.
> > 
> > Thanks,
> > Heming
> > 
> 
> You are absolutely correct. Theoretically, if the OCFS2_REFCOUNT_TREE_FL
> flag is set, the extent list should not be empty, and el->l_next_free_rec
> should be >= 1.
> 
> However, if the filesystem metadata is corrupted (e.g., due to bit rot or
> software bugs), it is possible that l_next_free_rec reads as 0 even for a
> tree root. In the current implementation, such inconsistency leads to a
> NULL pointer dereference and panics the system.
> 
> This patch is intended as a hardening measure. Instead of crashing the
> kernel when encountering such invalid on-disk data, it is safer to report
> a filesystem error via ocfs2_error and abort the operation gracefully.
> 
> This approach aligns with recent fixes in OCFS2. For example, Commit
> 44acc46d182f ("ocfs2: avoid NULL pointer dereference in
> dx_dir_lookup_rec()") addressed an identical issue where an empty extent
> list left the 'rec' pointer NULL.
> 
> I will update the commit message in the next version to explicitly state
> that this is a hardening measure against on-disk corruption, ensuring the
> rationale is accurate.

I am quoting the rule from Documentation/process/stable-kernel-rules.rst:

Rules on what kind of patches are accepted, and which ones are not, into the
"-stable" tree:
  ... ...
  - No "This could be a problem..." type of things like a "theoretical race
    condition", unless an explanation of how the bug can be exploited is also
    provided.

This principle applies to your case as well (even though you are targeting
the mainline tree). I don't believe the issues mentioned in these patches
are worth fixing without proof. Please provide reproduction steps for all
your patches before we continue the review.

Thanks,
Heming

> 
> >> 
> >> This patch adds an 'else' branch to the 'if (found)' check. If no valid
> >> record is found, it reports a filesystem error and exits, preventing
> >> the invalid memory access.
> >> 
> >> Fixes: e73a819db9c2 ("ocfs2: Add support for incrementing refcount in the tree.")
> >> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@...il.com>
> >> ---
> >> Changelog:
> >> 
> >> v1 -> v2:
> >> 
> >> 1. Add a Fixes tag.
> >> ---
> >>  fs/ocfs2/refcounttree.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> >> index c92e0ea85bca..464bdd6e0a8e 100644
> >> --- a/fs/ocfs2/refcounttree.c
> >> +++ b/fs/ocfs2/refcounttree.c
> >> @@ -1122,6 +1122,11 @@ static int ocfs2_get_refcount_rec(struct ocfs2_caching_info *ci,
> >>  
> >>  		if (cpos_end < low_cpos + len)
> >>  			len = cpos_end - low_cpos;
> >> +	} else {
> >> +		ret = ocfs2_error(sb, "Refcount tree %llu has no extent record covering cpos %u\n",
> >> +				  (unsigned long long)ocfs2_metadata_cache_owner(ci),
> >> +				  low_cpos);
> >> +		goto out;
> >>  	}
> >>  
> >>  	ret = ocfs2_read_refcount_block(ci, le64_to_cpu(rec->e_blkno),
> >> -- 
> >> 2.25.1
> >> 
> >>