Re: [PATCH 6.1 1/2] erofs: handle overlapped pclusters out of crafted images properly

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

On 2025/3/3 01:41, Gao Xiang wrote:
> Hi Fedor,
> 
> On 2025/3/2 18:56, Fedor Pchelkin wrote:
>> On Fri, 28. Feb 19:51, Alexey Panov wrote:
>>> From: Gao Xiang <hsiangkao@linux.alibaba.com>
>>>
>>> commit 9e2f9d34dd12e6e5b244ec488bcebd0c2d566c50 upstream.
>>>
>>> syzbot reported a task hang issue due to a deadlock case where it is
>>> waiting for the folio lock of a cached folio that will be used for
>>> cache I/Os.
>>>
>>> After looking into the crafted fuzzed image, I found it's formed with
>>> several overlapped big pclusters as below:
>>>
>>>   Ext:   logical offset   |  length :     physical offset    |  length
>>>     0:        0..   16384 |   16384 :     151552..    167936 |   16384
>>>     1:    16384..   32768 |   16384 :     155648..    172032 |   16384
>>>     2:    32768..   49152 |   16384 :  537223168.. 537239552 |   16384
>>> ...
>>>
>>> Here, extent 0/1 are physically overlapped although it's entirely
>>> _impossible_ for normal filesystem images generated by mkfs.
>>>
>>> First, managed folios containing compressed data will be marked as
>>> up-to-date and then unlocked immediately (unlike in-place folios) when
>>> compressed I/Os are complete.  If physical blocks are not submitted in
>>> the incremental order, there should be separate BIOs to avoid dependency
>>> issues.  However, the current code mis-arranges z_erofs_fill_bio_vec()
>>> and BIO submission which causes unexpected BIO waits.
>>>
>>> Second, managed folios will be connected to their own pclusters for
>>> efficient inter-queries.  However, this is somewhat hard to implement
>>> easily if overlapped big pclusters exist.  Again, these only appear in
>>> fuzzed images so let's simply fall back to temporary short-lived pages
>>> for correctness.
>>>
>>> Additionally, it justifies that referenced managed folios cannot be
>>> truncated for now and reverts part of commit 2080ca1ed3e4 ("erofs: tidy
>>> up `struct z_erofs_bvec`") for simplicity although it shouldn't be any
>>> difference.
>>>
>>> Reported-by: syzbot+4fc98ed414ae63d1ada2@syzkaller.appspotmail.com
>>> Reported-by: syzbot+de04e06b28cfecf2281c@syzkaller.appspotmail.com
>>> Reported-by: syzbot+c8c8238b394be4a1087d@syzkaller.appspotmail.com
>>> Tested-by: syzbot+4fc98ed414ae63d1ada2@syzkaller.appspotmail.com
>>> Closes: https://lore.kernel.org/r/0000000000002fda01061e334873@google.com
>>> Fixes: 8e6c8fa9f2e9 ("erofs: enable big pcluster feature")
>>> Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
>>> Link: https://lore.kernel.org/r/20240910070847.3356592-1-hsiangkao@linux.alibaba.com
>>> [Alexey: minor fix to resolve merge conflict]
>>
>> Urgh, it doesn't look so minor indeed. Backward struct folio -> struct
>> page conversions can be tricky sometimes. Please see several comments
>> below.
> 
> I manually backported it for Linux 6.6.y, see
> https://web.git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-6.6.y&id=1bf7e414cac303c9aec1be67872e19be8b64980c
> 
> Actually I had a very similiar backport for Linux 6.1.y,
> but I forgot to send it out due to other ongoing stuffs.
> 
> I think this backport patch is all good, but you could
> also mention it follows linux 6.6.y conflict changes
> instead of "minor fix to resolve merge conflict".
> 
>>
>>> Signed-off-by: Alexey Panov <apanov@astralinux.ru>
>>> ---
>>> Backport fix for CVE-2024-47736
>>>
>>>   fs/erofs/zdata.c | 59 +++++++++++++++++++++++++-----------------------
>>>   1 file changed, 31 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
>>> index 94e9e0bf3bbd..ac01c0ede7f7 100644
>>
>> I'm looking at the diff of upstream commit and the first thing it does
>> is to remove zeroing out the folio/page private field here:
>>
>>    // upstream commit 9e2f9d34dd12 ("erofs: handle overlapped pclusters out of crafted images properly")
>>    @@ -1450,7 +1451,6 @@ static void z_erofs_fill_bio_vec(struct bio_vec *bvec,
>>             * file-backed folios will be used instead.
>>             */
>>            if (folio->private == (void *)Z_EROFS_PREALLOCATED_PAGE) {
>>    -               folio->private = 0;
>>                    tocache = true;
>>                    goto out_tocache;
>>            }
>>
>> while in 6.1.129 the corresponding fragment seems untouched with the
>> backport patch. Is it intended?
> 
> Yes, because it was added in
> commit 2080ca1ed3e4 ("erofs: tidy up `struct z_erofs_bvec`")
> and dropped again.
> 
> But for Linux 6.6.y and 6.1.y, we don't need to backport
> 2080ca1ed3e4.

Oh, it seems that I missed this part when backporting
for 6.6.y, but it has no actual difference because
`page->private` will be updated in `goto out_tocache`.

so `set_page_private(page, 0);` was actual a redundant
logic, you could follow the upstream to discard
`set_page_private(page, 0);`.

Thanks,
Gao Xiang