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

[Utkal Singh]

Hi Gao Xiang,

Thank you for the review.

Understood, I will include a reproducible way in v2 for both patches.

I have also created a GitHub issue to track these patches:
https://github.com/erofs/erofs-utils/issues/47

v2 will follow shortly.

Best regards,
Utkal Singh

On Tue, 17 Mar 2026 at 08:10, Gao Xiang <hsiangkao at linux.alibaba.com> wrote:

>
>
> On 2026/3/17 10:16, Gao Xiang wrote:
> >
> >
> > 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.
>
> Nevertheless, I'm fine if we try to harden this part, but the commit
> message should clarify the impact: it actually has no stability impact
> out of those images.
>
> Also there are many threads with your contribution, it's hard for me
> to follow those threads, now.
>
> Would you mind raising a github issue
> https://github.com/erofs/erofs-utils/issues
>
> and list all your patches for merging (with meaningful topics are
> preferred) ?
>
> >
> >>
> >> 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 at 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);
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20260317/7c88afa9/attachment.htm>