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

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

[Utkal Singh]

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.

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.

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

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)) {
+		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);
-- 
2.43.0

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

[Gao Xiang]

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);

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

[Gao Xiang]

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@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);
> 

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

[Utkal Singh]

[-- Attachment #1: Type: text/plain, Size: 3493 bytes --]

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@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@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);
> >
>
>

[-- Attachment #2: Type: text/html, Size: 4819 bytes --]

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

[Utkal Singh]

erofs_init_inode_xattrs() reads h_shared_count from the on-disk xattr
ibody header and uses it to size a malloc and drive a loop that reads
shared xattr IDs.  If h_shared_count exceeds the space available
within xattr_isize, the loop reads past the intended ibody region
and the malloc is oversized.

Validate that h_shared_count does not exceed the number of __le32
entries that fit after the ibody header.  Return -EFSCORRUPTED with
a diagnostic message on failure.

Reproducer:
  mkdir testdir && echo hello > testdir/a.txt
  setfattr -n user.test -v val testdir/a.txt
  mkfs.erofs test.img testdir
  # corrupt h_shared_count (offset = nid*32 + inode_size + 4) to 0xFF
  # then: fsck.erofs --extract=/tmp/out --xattrs test_corrupted.img
  # Without patch: silently processes invalid shared xattr IDs
  # With patch: returns -EFSCORRUPTED

Signed-off-by: Utkal Singh <singhutkal015@gmail.com>
---
 lib/xattr.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/xattr.c b/lib/xattr.c
index 565070a..5888602 100644
--- a/lib/xattr.c
+++ b/lib/xattr.c
@@ -1182,6 +1182,14 @@ static int erofs_init_inode_xattrs(struct erofs_inode *vi)
 
 	ih = it.kaddr;
 	vi->xattr_shared_count = ih->h_shared_count;
+	if (vi->xattr_shared_count >
+	    (vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)) /
+	    sizeof(__le32)) {
+		erofs_err("invalid h_shared_count %u in nid %llu",
+			  vi->xattr_shared_count, 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);
-- 
2.43.0

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

[Gao Xiang]

On 2026/3/17 19:17, Utkal Singh wrote:
> erofs_init_inode_xattrs() reads h_shared_count from the on-disk xattr
> ibody header and uses it to size a malloc and drive a loop that reads
> shared xattr IDs.  If h_shared_count exceeds the space available
> within xattr_isize, the loop reads past the intended ibody region
> and the malloc is oversized.
> 
> Validate that h_shared_count does not exceed the number of __le32
> entries that fit after the ibody header.  Return -EFSCORRUPTED with
> a diagnostic message on failure.
> 
> Reproducer:
>    mkdir testdir && echo hello > testdir/a.txt
>    setfattr -n user.test -v val testdir/a.txt
>    mkfs.erofs test.img testdir
>    # corrupt h_shared_count (offset = nid*32 + inode_size + 4) to 0xFF
>    # then: fsck.erofs --extract=/tmp/out --xattrs test_corrupted.img
>    # Without patch: silently processes invalid shared xattr IDs
>    # With patch: returns -EFSCORRUPTED
> 
> Signed-off-by: Utkal Singh <singhutkal015@gmail.com>
> ---
>   lib/xattr.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/lib/xattr.c b/lib/xattr.c
> index 565070a..5888602 100644
> --- a/lib/xattr.c
> +++ b/lib/xattr.c
> @@ -1182,6 +1182,14 @@ static int erofs_init_inode_xattrs(struct erofs_inode *vi)
>   
>   	ih = it.kaddr;
>   	vi->xattr_shared_count = ih->h_shared_count;
> +	if (vi->xattr_shared_count >
> +	    (vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)) /
> +	    sizeof(__le32)) {
> +		erofs_err("invalid h_shared_count %u in nid %llu",
> +			  vi->xattr_shared_count, vi->nid | 0ULL);
> +		erofs_put_metabuf(&it.buf);
> +		return -EFSCORRUPTED;
> +	}

Again, why not `vi->xattr_shared_count * sizeof(__le32) >
	vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)`?

Thanks,
Gao Xiang

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

[Utkal Singh]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

Hi Gao Xiang,

Thanks for the review. I just sent v3 which uses the multiplication form as
you suggested.

Best regards, Utkal Singh

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

>
>
> On 2026/3/17 19:17, Utkal Singh wrote:
> > erofs_init_inode_xattrs() reads h_shared_count from the on-disk xattr
> > ibody header and uses it to size a malloc and drive a loop that reads
> > shared xattr IDs.  If h_shared_count exceeds the space available
> > within xattr_isize, the loop reads past the intended ibody region
> > and the malloc is oversized.
> >
> > Validate that h_shared_count does not exceed the number of __le32
> > entries that fit after the ibody header.  Return -EFSCORRUPTED with
> > a diagnostic message on failure.
> >
> > Reproducer:
> >    mkdir testdir && echo hello > testdir/a.txt
> >    setfattr -n user.test -v val testdir/a.txt
> >    mkfs.erofs test.img testdir
> >    # corrupt h_shared_count (offset = nid*32 + inode_size + 4) to 0xFF
> >    # then: fsck.erofs --extract=/tmp/out --xattrs test_corrupted.img
> >    # Without patch: silently processes invalid shared xattr IDs
> >    # With patch: returns -EFSCORRUPTED
> >
> > Signed-off-by: Utkal Singh <singhutkal015@gmail.com>
> > ---
> >   lib/xattr.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/lib/xattr.c b/lib/xattr.c
> > index 565070a..5888602 100644
> > --- a/lib/xattr.c
> > +++ b/lib/xattr.c
> > @@ -1182,6 +1182,14 @@ static int erofs_init_inode_xattrs(struct
> erofs_inode *vi)
> >
> >       ih = it.kaddr;
> >       vi->xattr_shared_count = ih->h_shared_count;
> > +     if (vi->xattr_shared_count >
> > +         (vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)) /
> > +         sizeof(__le32)) {
> > +             erofs_err("invalid h_shared_count %u in nid %llu",
> > +                       vi->xattr_shared_count, vi->nid | 0ULL);
> > +             erofs_put_metabuf(&it.buf);
> > +             return -EFSCORRUPTED;
> > +     }
>
> Again, why not `vi->xattr_shared_count * sizeof(__le32) >
>         vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)`?
>
> Thanks,
> Gao Xiang
>

[-- Attachment #2: Type: text/html, Size: 2961 bytes --]