When walking through an inode extents, the ext4_ext_binsearch_idx() function
assumes that the extent header has been previously validated. However, there
are no checks that verify that the number of entries (eh->eh_entries) is
non-zero when depth is > 0. And this will lead to problems because the
EXT_FIRST_INDEX() and EXT_LAST_INDEX() will return garbage and result in this:
[ 135.245946] ------------[ cut here ]------------
[ 135.247579] kernel BUG at fs/ext4/extents.c:2258!
[ 135.249045] invalid opcode: 0000 [#1] PREEMPT SMP
[ 135.250320] CPU: 2 PID: 238 Comm: tmp118 Not tainted 5.19.0-rc8+ #4
[ 135.252067] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
[ 135.255065] RIP: 0010:ext4_ext_map_blocks+0xc20/0xcb0
[ 135.256475] Code:
[ 135.261433] RSP: 0018:ffffc900005939f8 EFLAGS: 00010246
[ 135.262847] RAX: 0000000000000024 RBX: ffffc90000593b70 RCX: 0000000000000023
[ 135.264765] RDX: ffff8880038e5f10 RSI: 0000000000000003 RDI: ffff8880046e922c
[ 135.266670] RBP: ffff8880046e9348 R08: 0000000000000001 R09: ffff888002ca580c
[ 135.268576] R10: 0000000000002602 R11: 0000000000000000 R12: 0000000000000024
[ 135.270477] R13: 0000000000000000 R14: 0000000000000024 R15: 0000000000000000
[ 135.272394] FS: 00007fdabdc56740(0000) GS:ffff88807dd00000(0000) knlGS:0000000000000000
[ 135.274510] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 135.276075] CR2: 00007ffc26bd4f00 CR3: 0000000006261004 CR4: 0000000000170ea0
[ 135.277952] Call Trace:
[ 135.278635] <TASK>
[ 135.279247] ? preempt_count_add+0x6d/0xa0
[ 135.280358] ? percpu_counter_add_batch+0x55/0xb0
[ 135.281612] ? _raw_read_unlock+0x18/0x30
[ 135.282704] ext4_map_blocks+0x294/0x5a0
[ 135.283745] ? xa_load+0x6f/0xa0
[ 135.284562] ext4_mpage_readpages+0x3d6/0x770
[ 135.285646] read_pages+0x67/0x1d0
[ 135.286492] ? folio_add_lru+0x51/0x80
[ 135.287441] page_cache_ra_unbounded+0x124/0x170
[ 135.288510] filemap_get_pages+0x23d/0x5a0
[ 135.289457] ? path_openat+0xa72/0xdd0
[ 135.290332] filemap_read+0xbf/0x300
[ 135.291158] ? _raw_spin_lock_irqsave+0x17/0x40
[ 135.292192] new_sync_read+0x103/0x170
[ 135.293014] vfs_read+0x15d/0x180
[ 135.293745] ksys_read+0xa1/0xe0
[ 135.294461] do_syscall_64+0x3c/0x80
[ 135.295284] entry_SYSCALL_64_after_hwframe+0x46/0xb0
This patch simply adds an extra check in __ext4_ext_check(), verifying that
eh_entries is not 0 when eh_depth is > 0.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215941
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216283
Cc: Baokun Li <[email protected]>
Signed-off-by: Luís Henriques <[email protected]>
---
fs/ext4/extents.c | 5 +++++
1 file changed, 5 insertions(+)
Hi!
Baokun's feedback showed me that I had a partial understanding of the
problem. Thus, I'm sending v2 which pretty much uses Baokun's suggestion
and simplifies the solution. I've also added the link to the 2nd bugzilla
to the commit text.
Cheers,
--
Luís
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 53cfe2c681c4..a5457ac1999c 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -460,6 +460,11 @@ static int __ext4_ext_check(const char *function, unsigned int line,
error_msg = "invalid eh_entries";
goto corrupted;
}
+ if (unlikely((le16_to_cpu(eh->eh_entries) == 0) &&
+ (le16_to_cpu(eh->eh_depth > 0)))) {
+ error_msg = "eh_entries is 0 but eh_depth is > 0";
+ goto corrupted;
+ }
if (!ext4_valid_extent_entries(inode, eh, lblk, &pblk, depth)) {
error_msg = "invalid extent entries";
goto corrupted;
Hi Baokun!
On Fri, Aug 12, 2022 at 08:50:34PM +0800, Baokun Li wrote:
> Hi Lu?s,
...
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 53cfe2c681c4..a5457ac1999c 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -460,6 +460,11 @@ static int __ext4_ext_check(const char *function, unsigned int line,
> > error_msg = "invalid eh_entries";
> > goto corrupted;
> > }
> > + if (unlikely((le16_to_cpu(eh->eh_entries) == 0) &&
> > + (le16_to_cpu(eh->eh_depth > 0)))) {
>
> The parentheses are misplaced,
I'm not sure I understand what you mean. I want to have
if (unlikely((CONDITION A) && (CONDITION B))) {
/* ... */
}
so they look correct. Or is that a matter of style/alignment? (Which
checkpatch.pl doesn't complains about, by the way.)
>and le16_to_cpu is not needed here.
OK, I guess that, since both conditions do a comparison against '0', the
le16_to_cpu() can be dropped. And, if the parentheses problem you
mentioned above is a style problem, dropping it will also solve it because
that statement will become
if (unlikely((eh->eh_entries == 0) && (eh->eh_depth > 0))) {
/* ... */
}
And once again, thanks for your review!
Cheers,
--
Lu?s
>
> > + error_msg = "eh_entries is 0 but eh_depth is > 0";
> > + goto corrupted;
> > + }
> > if (!ext4_valid_extent_entries(inode, eh, lblk, &pblk, depth)) {
> > error_msg = "invalid extent entries";
> > goto corrupted;
> > .
>
> --
> With Best Regards,
> Baokun Li
>