Re: [PATCH] erofs-utils: lib: validate h_shared_count in erofs_init_inode_xattrs()

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On 2026/3/17 04:19, Utkal Singh wrote:
> erofs_init_inode_xattrs() reads h_shared_count directly from the on-disk
> xattr ibody header and uses it to size a heap allocation and drive a
> read loop without checking whether the implied shared xattr array fits
> within xattr_isize.
> 
> A crafted EROFS image with a large h_shared_count but a minimal
> xattr_isize causes the subsequent loop to read shared xattr entries
> beyond the xattr ibody boundary, interpreting unrelated on-disk data
> as shared xattr IDs.  This affects every library consumer -- dump.erofs,
> erofsfuse, and the rebuild path (lib/rebuild.c) -- none of which call
> the fsck-only erofs_verify_xattr() before reaching this code.

I don't think other than fsck tool, this must be checked, since it
won't cause any harmful behavior but the filesystem image is already
corrupted, and because of the corruption, the user should get the
corrupted result, but it still have no impact to the whole system
stablity.

> 
> Validate that h_shared_count fits within the available xattr body space
> before allocating or reading.  Use a division-based check to avoid any
> theoretical overflow in the multiplication.

I don't think it will overflow according to the ondisk format.

> 
> The subtraction is safe because callers above already reject
> xattr_isize < sizeof(struct erofs_xattr_ibody_header).

Please add a reproducible way.

> 
> Signed-off-by: Utkal Singh <singhutkal015@gmail.com>
> ---
>   lib/xattr.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/lib/xattr.c b/lib/xattr.c
> index 565070a..6891812 100644
> --- a/lib/xattr.c
> +++ b/lib/xattr.c
> @@ -1182,6 +1182,16 @@ static int erofs_init_inode_xattrs(struct erofs_inode *vi)
>   
>   	ih = it.kaddr;
>   	vi->xattr_shared_count = ih->h_shared_count;
> +	/* validate h_shared_count fits within xattr_isize */
> +	if (vi->xattr_shared_count >
> +	    (vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)) /
> +			sizeof(u32)) {

Can we avoid division?

> +		erofs_err("bogus h_shared_count %u (xattr_isize %u) @ nid %llu",
> +			  vi->xattr_shared_count, vi->xattr_isize,
> +			  vi->nid | 0ULL);
> +		erofs_put_metabuf(&it.buf);
> +		return -EFSCORRUPTED;
> +	}
>   	vi->xattr_shared_xattrs = malloc(vi->xattr_shared_count * sizeof(uint));
>   	if (!vi->xattr_shared_xattrs) {
>   		erofs_put_metabuf(&it.buf);