Re: [PATCH v2 3/3] ocfs2: deal with legacy signed xattr name hash values

[Joseph Qi]

On 10/9/26 4:45 PM, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] fs/ocfs2: NULL pointer dereference in ocfs2_xattr_set_entry_bucket() on corrupted index
> --
> 
> commit d720400774af2e4eeb97fd62736805be315e728c
> Author: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
> 
> ocfs2: deal with legacy signed xattr name hash values
> 
> This commit introduces retry logic to xattr tree searches to fall back to a
> legacy signed hash if an entry is missed using the unsigned hash. This fixes
> a regression where names containing bytes >= 0x80 became unreachable when
> char signedness was changed in kbuild.
> 
>> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
>> index c5a39a7d43d07..e92bcf401490d 100644
>> --- a/fs/ocfs2/xattr.c
>> +++ b/fs/ocfs2/xattr.c
> 
> [ ... ]
> 
>> @@ -4304,11 +4322,12 @@ static int ocfs2_xattr_bucket_find(struct inode *inode,
>>  	return ret;
>>  }
>>  
>> -static int ocfs2_xattr_index_block_find(struct inode *inode,
>> -					struct buffer_head *root_bh,
>> -					int name_index,
>> -					const char *name,
>> -					struct ocfs2_xattr_search *xs)
>> +static int __ocfs2_xattr_index_block_find(struct inode *inode,
>> +					  struct buffer_head *root_bh,
>> +					  int name_index,
>> +					  const char *name,
>> +					  u32 name_hash,
>> +					  struct ocfs2_xattr_search *xs)
>>  {
>>  	int ret;
>>  	struct ocfs2_xattr_block *xb =
>> @@ -4317,7 +4336,6 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
>>  	struct ocfs2_extent_list *el = &xb_root->xt_list;
>>  	u64 p_blkno = 0;
>>  	u32 first_hash, num_clusters = 0;
>> -	u32 name_hash = ocfs2_xattr_name_hash(inode, name, strlen(name));
>>  
>>  	if (le16_to_cpu(el->l_next_free_rec) == 0)
>>  		return -ENODATA;
> 
> [ ... ]
> 
>> @@ -4348,6 +4366,59 @@ static int ocfs2_xattr_index_block_find(struct inode *inode,
>>  	return ret;
>>  }
>>  
>> +static int ocfs2_xattr_index_block_find(struct inode *inode,
>> +					struct buffer_head *root_bh,
>> +					int name_index,
>> +					const char *name,
>> +					struct ocfs2_xattr_search *xs)
>> +{
>> +	u32 name_hash, legacy_hash;
>> +	int name_len = strlen(name);
>> +	int ret;
>> +
>> +	name_hash = ocfs2_xattr_name_hash(inode, name, name_len);
>> +
>> +	ret = __ocfs2_xattr_index_block_find(inode, root_bh, name_index, name,
>> +					     name_hash, xs);
>> +	if (ret != -ENODATA)
>> +		return ret;
>> +
>> +	/*
>> +	 * Nothing under the current hash.  The entry may have been stored by
>> +	 * an older kernel, which sign-extended the name bytes when hashing.
>> +	 * Skip the retry when the two hashes are equal, so that a name made
>> +	 * only of ASCII does not have to walk the tree twice.
>> +	 */
>> +	legacy_hash = ocfs2_xattr_name_hash_signed(inode, name, name_len);
>> +	if (legacy_hash == name_hash)
>> +		return ret;
>> +
>> +	/*
>> +	 * A miss still leaves xs->bucket holding the bucket a new entry would
>> +	 * be inserted into, so drop it before searching again.
>> +	 */
>> +	ocfs2_xattr_bucket_relse(xs->bucket);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> [Severity: High]
> Does this new retry logic need to validate that xs->bucket was actually
> populated before relying on it?
> 
> The patch builds its retry logic under the explicit assumption that a miss
> invariably leaves xs->bucket populated. However, if the filesystem is
> corrupted such that the external xattr block is indexed but its extent list
> is empty (el->l_next_free_rec == 0), __ocfs2_xattr_index_block_find()
> returns -ENODATA immediately without calling ocfs2_xattr_bucket_find().
> 
> This early return leaves xs->bucket->bu_bhs unpopulated.
> 
> Later, during a setxattr() operation, ocfs2_xattr_block_set() delegates
> to ocfs2_xattr_set_entry_index_block() and then
> ocfs2_xattr_set_entry_bucket(). The call to ocfs2_init_xattr_bucket_xa_loc()
> expands the bucket_xh() macro, which accesses bucket->bu_bhs[0]->b_data.
> Since bu_bhs[0] is unpopulated, this triggers a NULL pointer dereference.
> 
> Can we add validation to handle cases where __ocfs2_xattr_index_block_find()
> returns -ENODATA but doesn't actually populate the bucket?
> 

It's a pre-exsiting issue.
It seems we have to enhance the validation in ocfs2_validate_xattr_block().

Thanks,
Joseph