<div dir="ltr">Hi Gao Xiang,<br><br>Thank you for the review.<br><br>Understood, I will include a reproducible way in v2 for both patches.<br><br>I have also created a GitHub issue to track these patches:<br><a href="https://github.com/erofs/erofs-utils/issues/47">https://github.com/erofs/erofs-utils/issues/47</a><br><br>v2 will follow shortly.<br><br>Best regards,<br>Utkal Singh</div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, 17 Mar 2026 at 08:10, Gao Xiang <<a href="mailto:hsiangkao@linux.alibaba.com">hsiangkao@linux.alibaba.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 2026/3/17 10:16, Gao Xiang wrote:<br>
> <br>
> <br>
> On 2026/3/17 04:19, Utkal Singh wrote:<br>
>> erofs_init_inode_xattrs() reads h_shared_count directly from the on-disk<br>
>> xattr ibody header and uses it to size a heap allocation and drive a<br>
>> read loop without checking whether the implied shared xattr array fits<br>
>> within xattr_isize.<br>
>><br>
>> A crafted EROFS image with a large h_shared_count but a minimal<br>
>> xattr_isize causes the subsequent loop to read shared xattr entries<br>
>> beyond the xattr ibody boundary, interpreting unrelated on-disk data<br>
>> as shared xattr IDs.  This affects every library consumer -- dump.erofs,<br>
>> erofsfuse, and the rebuild path (lib/rebuild.c) -- none of which call<br>
>> the fsck-only erofs_verify_xattr() before reaching this code.<br>
> <br>
> I don't think other than fsck tool, this must be checked, since it<br>
> won't cause any harmful behavior but the filesystem image is already<br>
> corrupted, and because of the corruption, the user should get the<br>
> corrupted result, but it still have no impact to the whole system<br>
> stablity.<br>
<br>
Nevertheless, I'm fine if we try to harden this part, but the commit<br>
message should clarify the impact: it actually has no stability impact<br>
out of those images.<br>
<br>
Also there are many threads with your contribution, it's hard for me<br>
to follow those threads, now.<br>
<br>
Would you mind raising a github issue<br>
<a href="https://github.com/erofs/erofs-utils/issues" rel="noreferrer" target="_blank">https://github.com/erofs/erofs-utils/issues</a><br>
<br>
and list all your patches for merging (with meaningful topics are<br>
preferred) ?<br>
<br>
> <br>
>><br>
>> Validate that h_shared_count fits within the available xattr body space<br>
>> before allocating or reading.  Use a division-based check to avoid any<br>
>> theoretical overflow in the multiplication.<br>
> <br>
> I don't think it will overflow according to the ondisk format.<br>
> <br>
>><br>
>> The subtraction is safe because callers above already reject<br>
>> xattr_isize < sizeof(struct erofs_xattr_ibody_header).<br>
> <br>
> Please add a reproducible way.<br>
> <br>
>><br>
>> Signed-off-by: Utkal Singh <<a href="mailto:singhutkal015@gmail.com" target="_blank">singhutkal015@gmail.com</a>><br>
>> ---<br>
>>   lib/xattr.c | 10 ++++++++++<br>
>>   1 file changed, 10 insertions(+)<br>
>><br>
>> diff --git a/lib/xattr.c b/lib/xattr.c<br>
>> index 565070a..6891812 100644<br>
>> --- a/lib/xattr.c<br>
>> +++ b/lib/xattr.c<br>
>> @@ -1182,6 +1182,16 @@ static int erofs_init_inode_xattrs(struct erofs_inode *vi)<br>
>>       ih = it.kaddr;<br>
>>       vi->xattr_shared_count = ih->h_shared_count;<br>
>> +    /* validate h_shared_count fits within xattr_isize */<br>
>> +    if (vi->xattr_shared_count ><br>
>> +        (vi->xattr_isize - sizeof(struct erofs_xattr_ibody_header)) /<br>
>> +            sizeof(u32)) {<br>
> <br>
> Can we avoid division?<br>
> <br>
>> +        erofs_err("bogus h_shared_count %u (xattr_isize %u) @ nid %llu",<br>
>> +              vi->xattr_shared_count, vi->xattr_isize,<br>
>> +              vi->nid | 0ULL);<br>
>> +        erofs_put_metabuf(&it.buf);<br>
>> +        return -EFSCORRUPTED;<br>
>> +    }<br>
>>       vi->xattr_shared_xattrs = malloc(vi->xattr_shared_count * sizeof(uint));<br>
>>       if (!vi->xattr_shared_xattrs) {<br>
>>           erofs_put_metabuf(&it.buf);<br>
> <br>
<br>
</blockquote></div>