syzbot reported a use-after-free Read in ext4_find_extent that is hit when
using a corrupted file system. The bug was reported on Android 5.15, but
using the same reproducer triggers the bug on v6.2-rc1 as well.
Fix the use-after-free by checking the extent header magic. An alternative
would be to check the values of EXT4_{FIRST,LAST}_{EXTENT,INDEX} used in
ext4_ext_binsearch() and ext4_ext_binsearch_idx(), so that we make sure
that pointers returned by EXT4_{FIRST,LAST}_{EXTENT,INDEX} don't exceed the
bounds of the extent tree node. But this alternative will not squash
the bug for the cases where eh->eh_entries fit into eh->eh_max. We could
also try to check the sanity of the path, but costs more than checking just
the header magic, so stick to the header magic sanity check.
Link: https://syzkaller.appspot.com/bug?id=be6e90ce70987950e6deb3bac8418344ca8b96cd
Reported-by: [email protected]
Cc: [email protected]
Signed-off-by: Tudor Ambarus <[email protected]>
---
v2: drop wrong/uneeded le16_to_cpu() conversion for eh->eh_magic
fs/ext4/extents.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9de1c9d1a13d..bedc8c098449 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -894,6 +894,12 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
gfp_flags |= __GFP_NOFAIL;
eh = ext_inode_hdr(inode);
+ if (eh->eh_magic != EXT4_EXT_MAGIC) {
+ EXT4_ERROR_INODE(inode, "Extent header has invalid magic.");
+ ret = -EFSCORRUPTED;
+ goto err;
+ }
+
depth = ext_depth(inode);
if (depth < 0 || depth > EXT4_MAX_EXTENT_DEPTH) {
EXT4_ERROR_INODE(inode, "inode has invalid extent depth: %d",
--
2.34.1
On Fri, Dec 30, 2022 at 08:29:31AM +0200, Tudor Ambarus wrote:
> syzbot reported a use-after-free Read in ext4_find_extent that is hit when
> using a corrupted file system. The bug was reported on Android 5.15, but
> using the same reproducer triggers the bug on v6.2-rc1 as well.
>
> Fix the use-after-free by checking the extent header magic. An alternative
> would be to check the values of EXT4_{FIRST,LAST}_{EXTENT,INDEX} used in
> ext4_ext_binsearch() and ext4_ext_binsearch_idx(), so that we make sure
> that pointers returned by EXT4_{FIRST,LAST}_{EXTENT,INDEX} don't exceed the
> bounds of the extent tree node. But this alternative will not squash
> the bug for the cases where eh->eh_entries fit into eh->eh_max. We could
> also try to check the sanity of the path, but costs more than checking just
> the header magic, so stick to the header magic sanity check.
>
> Link: https://syzkaller.appspot.com/bug?id=be6e90ce70987950e6deb3bac8418344ca8b96cd
> Reported-by: [email protected]
> Cc: [email protected]
> Signed-off-by: Tudor Ambarus <[email protected]>
> ---
> v2: drop wrong/uneeded le16_to_cpu() conversion for eh->eh_magic
>
> fs/ext4/extents.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 9de1c9d1a13d..bedc8c098449 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -894,6 +894,12 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
> gfp_flags |= __GFP_NOFAIL;
>
> eh = ext_inode_hdr(inode);
> + if (eh->eh_magic != EXT4_EXT_MAGIC) {
> + EXT4_ERROR_INODE(inode, "Extent header has invalid magic.");
> + ret = -EFSCORRUPTED;
> + goto err;
> + }
> +
This is (incompletely) validating the extent header in the inode. Isn't that
supposed to happen when the inode is loaded? See how __ext4_iget() calls
ext4_ext_check_inode(). Why isn't that working here?
- Eric
Hi, Eric,
On 30.12.2022 08:56, Eric Biggers wrote:
> On Fri, Dec 30, 2022 at 08:29:31AM +0200, Tudor Ambarus wrote:
>> syzbot reported a use-after-free Read in ext4_find_extent that is hit when
>> using a corrupted file system. The bug was reported on Android 5.15, but
>> using the same reproducer triggers the bug on v6.2-rc1 as well.
>>
>> Fix the use-after-free by checking the extent header magic. An alternative
>> would be to check the values of EXT4_{FIRST,LAST}_{EXTENT,INDEX} used in
>> ext4_ext_binsearch() and ext4_ext_binsearch_idx(), so that we make sure
>> that pointers returned by EXT4_{FIRST,LAST}_{EXTENT,INDEX} don't exceed the
>> bounds of the extent tree node. But this alternative will not squash
>> the bug for the cases where eh->eh_entries fit into eh->eh_max. We could
>> also try to check the sanity of the path, but costs more than checking just
>> the header magic, so stick to the header magic sanity check.
>>
>> Link: https://syzkaller.appspot.com/bug?id=be6e90ce70987950e6deb3bac8418344ca8b96cd
>> Reported-by: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Tudor Ambarus <[email protected]>
>> ---
>> v2: drop wrong/uneeded le16_to_cpu() conversion for eh->eh_magic
>>
>> fs/ext4/extents.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 9de1c9d1a13d..bedc8c098449 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -894,6 +894,12 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>> gfp_flags |= __GFP_NOFAIL;
>>
>> eh = ext_inode_hdr(inode);
>> + if (eh->eh_magic != EXT4_EXT_MAGIC) {
>> + EXT4_ERROR_INODE(inode, "Extent header has invalid magic.");
>> + ret = -EFSCORRUPTED;
>> + goto err;
>> + }
>> +
>
> This is (incompletely) validating the extent header in the inode. Isn't that
> supposed to happen when the inode is loaded? See how __ext4_iget() calls
> ext4_ext_check_inode(). Why isn't that working here?
>
Seems that __ext4_iget() is not called on writes. You can find below the
sequence of calls that leads to the bug. The debug was done on v6.2-rc1.
I assume the extents check is no longer done on writes since
commit 7a262f7c69163cd4811f2f838faef5c5b18439c9. The commit doesn't
specify the reason though.
Cheers,
ta
---
#20 0xffffffff85000087 in entry_SYSCALL_64 ()
at /home/atudor/work/aosp/arch/x86/entry/entry_64.S:120
#19 do_syscall_64 (regs=0xffffc90001fbff58, nr=82542)
at /home/atudor/work/aosp/arch/x86/entry/common.c:80
#18 0xffffffff84f27caf in do_syscall_x64 (regs=0xffffc90001fbff58, nr=82542)
at /home/atudor/work/aosp/arch/x86/entry/common.c:50
#17 __x64_sys_write (regs=<optimized out>) at
/home/atudor/work/aosp/fs/read_write.c:646
#16 __se_sys_write (fd=4, buf=536871424, count=0)
at /home/atudor/work/aosp/fs/read_write.c:646
#15 0xffffffff81c668fb in __do_sys_write (fd=4, buf=0x20000200
"threaded", count=0)
at /home/atudor/work/aosp/fs/read_write.c:649
#14 0xffffffff81c66758 in ksys_write (fd=fd@entry=4,
buf=buf@entry=0x20000200 "threaded",
count=392007683) at /home/atudor/work/aosp/fs/read_write.c:637
#13 vfs_write (file=file@entry=0xffff8881352f9900, buf=<optimized out>,
buf@entry=0x20000200 "threaded", count=<optimized out>,
count@entry=392007683,
pos=<optimized out>, pos@entry=0xffffc90001fbfe80)
at /home/atudor/work/aosp/fs/read_write.c:584
#12 new_sync_write (filp=<optimized out>, buf=0x20000200 "threaded",
len=392007683,
ppos=0xffffc90001fbfe80) at /home/atudor/work/aosp/fs/read_write.c:491
#11 0xffffffff81c65ba5 in call_write_iter (file=0xffff8881352f9900,
kio=<optimized out>,
iter=<optimized out>) at /home/atudor/work/aosp/include/linux/fs.h:2186
#10 0xffffffff81f11e78 in ext4_file_write_iter (iocb=<optimized out>,
from=0xffffc90001fbfd70) at /home/atudor/work/aosp/fs/ext4/file.c:700
#9 0xffffffff81f148ff in ext4_buffered_write_iter (iocb=0xffffc90001fbfd20,
from=from@entry=0xffffc90001fbfd70) at
/home/atudor/work/aosp/fs/ext4/file.c:285
#8 0xffffffff81a4aef4 in generic_perform_write
(iocb=iocb@entry=0xffffc90001fbfd20,
i=<optimized out>, i@entry=0xffffc90001fbfd70)
at /home/atudor/work/aosp/mm/filemap.c:3772
#7 0xffffffff81f682b9 in ext4_da_write_begin (file=<optimized out>,
mapping=<optimized out>, pos=<optimized out>, len=<optimized out>,
pagep=<optimized out>, fsdata=<optimized out>)
at /home/atudor/work/aosp/fs/ext4/inode.c:3082
#6 0xffffffff81f66995 in ext4_block_write_begin
(page=page@entry=0xffffea0004859500,
pos=pos@entry=32768, len=len@entry=4096,
get_block=0xffffffff81f47b10 <ext4_da_get_block_prep>)
at /home/atudor/work/aosp/fs/ext4/inode.c:1098
#5 ext4_da_get_block_prep (inode=0xffff888117bbe7a8, iblock=16,
bh=0xffff888117b5db28,
create=<optimized out>) at /home/atudor/work/aosp/fs/ext4/inode.c:1870
#4 ext4_da_map_blocks (inode=0xffff888117bbe7a8, iblock=16,
bh=0xffff888117b5db28,
map=<optimized out>) at /home/atudor/work/aosp/fs/ext4/inode.c:1806
#3 0xffffffff81f48526 in ext4_insert_delayed_block
(inode=0xffff888117bbe7a8,
lblk=<optimized out>) at /home/atudor/work/aosp/fs/ext4/inode.c:1696
#2 0xffffffff81ef9b96 in ext4_clu_mapped
(inode=inode@entry=0xffff888117bbe7a8, lclu=1)
at /home/atudor/work/aosp/fs/ext4/extents.c:5809
#1 ext4_find_extent (inode=inode@entry=0xffff888117bbe7a8, block=16,
orig_path=orig_path@entry=0x0 <fixed_percpu_data>, flags=flags@entry=0)
at /home/atudor/work/aosp/fs/ext4/extents.c:931
#0 ext4_ext_binsearch_idx (path=0xffff888136ad3800, block=16,
inode=<optimized out>)
at /home/atudor/work/aosp/fs/ext4/extents.c:768
On Fri, Dec 30, 2022 at 01:42:45PM +0200, Tudor Ambarus wrote:
>
> Seems that __ext4_iget() is not called on writes.
It is called when the inode is first accessed. Usually that's when the file is
opened.
So the question is why didn't it validate the inode's extent header, or
alternatively how did the inode's extent header get corrupted afterwards.
> You can find below the sequence of calls that leads to the bug.
A stack trace is not a reproducer. Things must have happened before that point.
- Eric
On Fri, Dec 30, 2022 at 01:42:45PM +0200, Tudor Ambarus wrote:
> > This is (incompletely) validating the extent header in the inode. Isn't that
> > supposed to happen when the inode is loaded? See how __ext4_iget() calls
> > ext4_ext_check_inode(). Why isn't that working here?
> >
>
> Seems that __ext4_iget() is not called on writes. You can find below the
> sequence of calls that leads to the bug. The debug was done on v6.2-rc1.
> I assume the extents check is no longer done on writes since
> commit 7a262f7c69163cd4811f2f838faef5c5b18439c9. The commit doesn't
> specify the reason though.
Commit 7a262f7c6916 no longer does the check if the inode is already
in memory (which is the case when there is an open file descriptor).
That's because it should have been checked when it was first read into
memory.
So the the question is, did the inode get corrupted somehow after it
was read in from disk? If so that's the real problem, and that's what
needs to be root caused and fixed. It's not sufficient to just to
make the syzbot reproducer only longer reproduce. The question is
understanding what is fundamentally going on and fixing the real root
problem.
Regards,
- Ted
On 30.12.2022 22:07, Eric Biggers wrote:
> On Fri, Dec 30, 2022 at 01:42:45PM +0200, Tudor Ambarus wrote:
>>
>> Seems that __ext4_iget() is not called on writes.
>
> It is called when the inode is first accessed. Usually that's when the file is
> opened.
Okay, thanks.
>
> So the question is why didn't it validate the inode's extent header, or
> alternatively how did the inode's extent header get corrupted afterwards.
>
>> You can find below the sequence of calls that leads to the bug.
>
> A stack trace is not a reproducer. Things must have happened before that point.
>
I will try to dig more to understand what's happening. If you like to
take a look into the reproducer, here it is:
https://syzkaller.appspot.com/text?tag=ReproC&x=17beb560480000
The reproducer was used for Android 5.15 and the bug is reported at [1],
but as I mentioned earlier, using the same reproducer and config I hit
the bug on v6.2-rc1 as well.
Thanks for the help.
ta
[1]
https://syzkaller.appspot.com/bug?id=be6e90ce70987950e6deb3bac8418344ca8b96cd