Re: [PATCH v7 2/2] ocfs2: validate external xattr entries when reading metadata

On 7/5/26 10:53 AM, Cen Zhang wrote:
> ocfs2_validate_xattr_block() checks the xattr block header before the
> block reaches higher-level xattr users, but it does not verify that a
> non-indexed block's xh_count and entry offsets fit inside the block.
> Indexed buckets likewise reach list/get consumers after ECC without an
> entry-bounds check.
> 
> Use the flat xattr entry validator for non-indexed external xattr
> blocks, and use a bucket-specific validator for indexed buckets at
> metadata read time. The bucket validator keeps the entry array bounded
> by the first bucket block while checking name/value offsets against the
> bucket block they target.
> 
> Reject corrupted external xattr metadata before listxattr() or
> getxattr() can walk out-of-range entry arrays or name/value offsets.
> 
> Validation reproduced this kernel report:
> BUG: KASAN: use-after-free in ocfs2_xattr_list_entries+0xd7/0x190
> Read of size 1 at addr ffff88810a654007 by task ocfs2_xattr_lis/630
> Call Trace:
>   dump_stack_lvl+0x66/0xa0
>   print_report+0xce/0x630
>   kasan_report+0xe0/0x110
>   ocfs2_xattr_list_entries+0xd7/0x190
>   ocfs2_listxattr+0x3f6/0x610
>   listxattr+0x90/0xe0
>   path_listxattrat+0xed/0x220
>   do_syscall_64+0x115/0x6a0
>   entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Fixes: cf1d6c763fbc ("ocfs2: Add extended attribute support")
> Fixes: 0c044f0b24b9 ("ocfs2: Add xattr bucket iteration for large numbers of EAs")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Cen Zhang <[email protected]>

Looks fine.
Reviewed-by: Joseph Qi <[email protected]>
> ---
>  fs/ocfs2/xattr.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index ca76441625db..d23364c30e1a 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -390,6 +390,12 @@ static int ocfs2_init_xattr_bucket(struct ocfs2_xattr_bucket *bucket,
>  	return rc;
>  }
>  
> +static int ocfs2_validate_xattr_entries_flat(struct super_block *sb, u64 blkno,
> +					     struct ocfs2_xattr_header *xh,
> +					     size_t region_size);
> +static int ocfs2_validate_xattr_bucket(struct ocfs2_xattr_bucket *bucket,
> +				       u64 blkno);
> +
>  /* Read the xattr bucket at xb_blkno */
>  static int ocfs2_read_xattr_bucket(struct ocfs2_xattr_bucket *bucket,
>  				   u64 xb_blkno)
> @@ -408,6 +414,8 @@ static int ocfs2_read_xattr_bucket(struct ocfs2_xattr_bucket *bucket,
>  		spin_unlock(&OCFS2_SB(bucket->bu_inode->i_sb)->osb_xattr_lock);
>  		if (rc)
>  			mlog_errno(rc);
> +		else
> +			rc = ocfs2_validate_xattr_bucket(bucket, xb_blkno);
>  	}
>  
>  	if (rc)
> @@ -509,6 +517,22 @@ static int ocfs2_validate_xattr_block(struct super_block *sb,
>  				   le32_to_cpu(xb->xb_fs_generation));
>  	}
>  
> +	if (!(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED)) {
> +		size_t region_offset =
> +			offsetof(struct ocfs2_xattr_block, xb_attrs.xb_header);
> +
> +		if (bh->b_size < region_offset)
> +			return ocfs2_error(sb,
> +					   "Invalid xattr block %llu: block size %zu is too small\n",
> +					   (unsigned long long)bh->b_blocknr,
> +					   bh->b_size);
> +
> +		return ocfs2_validate_xattr_entries_flat(sb, bh->b_blocknr,
> +							 &xb->xb_attrs.xb_header,
> +							 bh->b_size -
> +							 region_offset);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1068,6 +1092,79 @@ int ocfs2_validate_inode_xattr(struct super_block *sb, u64 blkno,
>  	return ocfs2_validate_xattr_entries_flat(sb, blkno, xh, inline_size);
>  }
>  
> +static int ocfs2_validate_xattr_bucket(struct ocfs2_xattr_bucket *bucket,
> +				       u64 blkno)
> +{
> +	struct super_block *sb = bucket->bu_inode->i_sb;
> +	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
> +	u16 xattr_count = le16_to_cpu(xh->xh_count);
> +	size_t region_size = (size_t)sb->s_blocksize * bucket->bu_blocks;
> +	size_t entries_limit = sb->s_blocksize;
> +	size_t nv_limit = sb->s_blocksize;
> +	size_t max_entries;
> +	int i;
> +
> +	if (region_size < sizeof(*xh))
> +		return ocfs2_error(sb,
> +				   "Invalid xattr bucket %llu: region size %zu is too small\n",
> +				   (unsigned long long)blkno, region_size);
> +
> +	if (entries_limit < sizeof(*xh))
> +		return ocfs2_error(sb,
> +				   "Invalid xattr bucket %llu: entries limit %zu is too small\n",
> +				   (unsigned long long)blkno,
> +				   entries_limit);
> +
> +	max_entries = (entries_limit - sizeof(*xh)) /
> +		      sizeof(struct ocfs2_xattr_entry);
> +
> +	if (xattr_count > max_entries)
> +		return ocfs2_error(sb,
> +				   "Invalid xattr bucket %llu: entry count %u exceeds maximum %zu\n",
> +				   (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 block_off = name_offset >> sb->s_blocksize_bits;
> +		size_t block_offset = name_offset % nv_limit;
> +		size_t value_offset;
> +
> +		if (name_offset >= region_size || block_off >= bucket->bu_blocks)
> +			return ocfs2_error(sb,
> +					   "Invalid xattr bucket %llu: entry %d name is out of bounds\n",
> +					   (unsigned long long)blkno, i);
> +
> +		if (xe->xe_name_len > nv_limit - block_offset)
> +			return ocfs2_error(sb,
> +					   "Invalid xattr bucket %llu: entry %d name crosses block boundary\n",
> +					   (unsigned long long)blkno, i);
> +
> +		value_offset = block_offset + OCFS2_XATTR_SIZE(xe->xe_name_len);
> +		if (value_offset > nv_limit)
> +			return ocfs2_error(sb,
> +					   "Invalid xattr bucket %llu: entry %d value starts out of bounds\n",
> +					   (unsigned long long)blkno, i);
> +
> +		if (ocfs2_xattr_is_local(xe)) {
> +			if (le64_to_cpu(xe->xe_value_size) >
> +			    nv_limit - value_offset)
> +				return ocfs2_error(sb,
> +						   "Invalid xattr bucket %llu: entry %d value is out of bounds\n",
> +						   (unsigned long long)blkno,
> +						   i);
> +		} else if (sizeof(struct ocfs2_xattr_value_root) >
> +			   nv_limit - value_offset) {
> +			return ocfs2_error(sb,
> +					   "Invalid xattr bucket %llu: entry %d value root is out of bounds\n",
> +					   (unsigned long long)blkno, i);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int ocfs2_xattr_ibody_lookup_header(struct inode *inode,
>  					   struct ocfs2_dinode *di,
>  					   struct ocfs2_xattr_header **header)