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 for Android: free password hash cracker in your pocket

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

Message-Id: <20260119171215.1076-1-jiashengjiangcool@gmail.com>
Date: Mon, 19 Jan 2026 17:12:15 +0000
From: Jiasheng Jiang <jiashengjiangcool@...il.com>
To: heming.zhao@...e.com
Cc: jiashengjiangcool@...il.com,
	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, 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.

>> 
>> 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
>> 
>>