Re: [PATCH v4 1/2] ocfs2: validate inline xattrs during inode block validation

On 6/22/26 9:03 PM, Cen Zhang wrote:
> ocfs2_validate_inode_block() verifies a dinode before OCFS2 users walk
> metadata from it, but inline xattr metadata is still checked only in
> some consumers. The current ibody helper validates the inline header
> placement and entry count before deriving the header from
> i_xattr_inline_size, but it does not reject entry name/value bounds from
> inode block validation.
> 
> Extend the shared ibody helper with name/value bounds checks and call it
> from ocfs2_validate_inode_block(). Keep the get/list paths using the same
> helper before they derive pointers from i_xattr_inline_size so callers
> with an already-loaded dinode still get the same corruption check.
> 
> Reject corrupted inline xattr metadata before ocfs2_xattr_ibody_get() or
> listxattr() can walk past the inline storage.
> 
> Validation reproduced this kernel report:
> BUG: KASAN: use-after-free in ocfs2_xattr_find_entry+0x5a/0x170
> Read of size 2 at addr ffff8881242a2000 by task python3/529
> Call Trace:
>   dump_stack_lvl+0x66/0xa0
>   print_report+0xce/0x630
>   kasan_report+0xe0/0x110
>   ocfs2_xattr_find_entry+0x5a/0x170
>   ocfs2_xattr_get_nolock+0x20a/0x820
>   ocfs2_xattr_get+0x10c/0x1e0
>   __vfs_getxattr+0xe2/0x130
>   vfs_getxattr+0x185/0x1b0
> 
> Fixes: cf1d6c763fbc ("ocfs2: Add extended attribute support")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Cen Zhang <[email protected]>
> ---
>  fs/ocfs2/inode.c |   4 ++
>  fs/ocfs2/xattr.c | 112 +++++++++++++++++++++++++++++++++++++----------
>  fs/ocfs2/xattr.h |   2 +
>  3 files changed, 95 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
> index 662dbc845b8b..815bf3f659da 100644
> --- a/fs/ocfs2/inode.c
> +++ b/fs/ocfs2/inode.c
> @@ -1608,6 +1608,10 @@ int ocfs2_validate_inode_block(struct super_block *sb,
>  		goto bail;
>  	}
>  
> +	rc = ocfs2_validate_inode_xattr(sb, bh->b_blocknr, di);
> +	if (rc)
> +		goto bail;
> +
>  	if (le16_to_cpu(di->i_dyn_features) & OCFS2_INLINE_DATA_FL) {
>  		struct ocfs2_inline_data *data = &di->id2.i_data;
>  
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index fcddd3c13acd..00900e65634d 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -950,39 +950,105 @@ static int ocfs2_xattr_list_entries(struct inode *inode,
>  	return result;
>  }
>  
> -static int ocfs2_xattr_ibody_lookup_header(struct inode *inode,
> -					   struct ocfs2_dinode *di,
> -					   struct ocfs2_xattr_header **header)
> +static int ocfs2_validate_xattr_entries(struct super_block *sb, u64 blkno,
> +					struct ocfs2_xattr_header *xh,
> +					size_t storage_size,
> +					const char *where)

Seems we can define a ocfs2_xattr_entry_type and pass in, then compute
the size inside ocfs2_validate_xattr_entries().
e.g.

enum ocfs2_xattr_entry_type {
	OCFS2_XATTR_IBODY,
	OCFS2_XATTR_BLOCK,
	OCFS2_XATTR_BUCKET
};

And here 'where' is to identify inline xattr, xattr block or bucket.
Commonly, blkno is already sufficient to uniquely identifies the corrupt
block. So I'd rather drop it for simplification.

>  {
> -	u16 xattr_count;
> +	u16 xattr_count = le16_to_cpu(xh->xh_count);
>  	size_t max_entries;
> +	int i;
> +
> +	if (storage_size < sizeof(*xh))
> +		return ocfs2_error(sb,
> +				   "Invalid %s in block %llu: storage size %zu is too small\n",
> +				   where, (unsigned long long)blkno,
> +				   storage_size);
> +
> +	max_entries = (storage_size - sizeof(*xh)) /
> +		      sizeof(struct ocfs2_xattr_entry);
> +	if (xattr_count > max_entries) {
> +		return ocfs2_error(sb,
> +				   "Invalid %s in block %llu: entry count %u exceeds maximum %zu\n",
> +				   where, (unsigned long long)blkno,
> +				   xattr_count, max_entries);
> +	}
> +
> +	for (i = 0; i < xattr_count; i++) {
> +		struct ocfs2_xattr_entry *xe = &xh->xh_entries[i];
> +		size_t name_offset = le16_to_cpu(xe->xe_name_offset);
> +		size_t value_offset;
> +
> +		if (name_offset > storage_size ||
> +		    xe->xe_name_len > storage_size - name_offset)
> +			return ocfs2_error(sb,
> +					   "Invalid %s in block %llu: entry %d name is out of bounds\n",
> +					   where, (unsigned long long)blkno, i);
> +
> +		value_offset = name_offset + OCFS2_XATTR_SIZE(xe->xe_name_len);
> +		if (value_offset > storage_size)
> +			return ocfs2_error(sb,
> +					   "Invalid %s in block %llu: entry %d value starts out of bounds\n",
> +					   where, (unsigned long long)blkno, i);
> +
> +		if (ocfs2_xattr_is_local(xe)) {
> +			if (le64_to_cpu(xe->xe_value_size) >
> +			    storage_size - value_offset)
> +				return ocfs2_error(sb,
> +						   "Invalid %s in block %llu: entry %d value is out of bounds\n",
> +						   where,
> +						   (unsigned long long)blkno,
> +						   i);
> +		} else if (sizeof(struct ocfs2_xattr_value_root) >
> +			   storage_size - value_offset) {
> +			return ocfs2_error(sb,
> +					   "Invalid %s in block %llu: entry %d value root is out of bounds\n",
> +					   where, (unsigned long long)blkno, i);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ocfs2_validate_xattr_ibody_header(struct super_block *sb, u64 blkno,
> +					     struct ocfs2_dinode *di,
> +					     struct ocfs2_xattr_header **header)
> +{
> +	struct ocfs2_xattr_header *xh;
>  	u16 inline_size = le16_to_cpu(di->i_xattr_inline_size);
>  
> -	if (inline_size > inode->i_sb->s_blocksize ||
> +	if (inline_size > sb->s_blocksize ||
>  	    inline_size < sizeof(struct ocfs2_xattr_header)) {
> -		ocfs2_error(inode->i_sb,
> -			    "Invalid xattr inline size %u in inode %llu\n",
> -			    inline_size,
> -			    (unsigned long long)OCFS2_I(inode)->ip_blkno);
> -		return -EFSCORRUPTED;
> +		return ocfs2_error(sb,
> +				   "Invalid inode %llu: xattr inline size %u\n",
> +				   (unsigned long long)blkno, inline_size);
>  	}
>  
> -	*header = (struct ocfs2_xattr_header *)
> -		((void *)di + inode->i_sb->s_blocksize - inline_size);
> +	xh = (struct ocfs2_xattr_header *)
> +		((void *)di + sb->s_blocksize - inline_size);
> +	if (header)
> +		*header = xh;
>  
> -	xattr_count = le16_to_cpu((*header)->xh_count);
> -	max_entries = (inline_size - sizeof(struct ocfs2_xattr_header)) /
> -		      sizeof(struct ocfs2_xattr_entry);
> +	return ocfs2_validate_xattr_entries(sb, blkno, xh, inline_size,
> +					    "inline xattr");
> +}
>  
> -	if (xattr_count > max_entries) {
> -		ocfs2_error(inode->i_sb,
> -			    "xattr entry count %u exceeds maximum %zu in inode %llu\n",
> -			    xattr_count, max_entries,
> -			    (unsigned long long)OCFS2_I(inode)->ip_blkno);
> -		return -EFSCORRUPTED;
> -	}
> +int ocfs2_validate_inode_xattr(struct super_block *sb, u64 blkno,
> +			       struct ocfs2_dinode *di)
> +{
> +	if (!(le16_to_cpu(di->i_dyn_features) & OCFS2_INLINE_XATTR_FL))
> +		return 0;
>  
> -	return 0;
> +	return ocfs2_validate_xattr_ibody_header(sb, blkno, di, NULL);
> +}
> +
> +static int ocfs2_xattr_ibody_lookup_header(struct inode *inode,
> +					   struct ocfs2_dinode *di,
> +					   struct ocfs2_xattr_header **header)
> +{
> +	return ocfs2_validate_xattr_ibody_header(inode->i_sb,
> +						OCFS2_I(inode)->ip_blkno,
> +						di, header);
>  }

Since ocfs2_validate_inode_block() already validated at buffer read
time, it seems this per-operation re-check is wasted work.

Thanks,
Joseph

>  
>  int ocfs2_has_inline_xattr_value_outside(struct inode *inode,
> diff --git a/fs/ocfs2/xattr.h b/fs/ocfs2/xattr.h
> index 65e9aa743919..6b7589941315 100644
> --- a/fs/ocfs2/xattr.h
> +++ b/fs/ocfs2/xattr.h
> @@ -43,6 +43,8 @@ int ocfs2_xattr_set_handle(handle_t *, struct inode *, struct buffer_head *,
>  			   struct ocfs2_alloc_context *);
>  int ocfs2_has_inline_xattr_value_outside(struct inode *inode,
>  					 struct ocfs2_dinode *di);
> +int ocfs2_validate_inode_xattr(struct super_block *sb, u64 blkno,
> +			       struct ocfs2_dinode *di);
>  int ocfs2_xattr_remove(struct inode *, struct buffer_head *);
>  int ocfs2_init_security_get(struct inode *, struct inode *,
>  			    const struct qstr *,