* [PATCH v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0
@ 2022-08-12 10:53 Luís Henriques
2022-08-12 12:50 ` Baokun Li
0 siblings, 1 reply; 4+ messages in thread
From: Luís Henriques @ 2022-08-12 10:53 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger
Cc: wenqingliu0120, linux-ext4, linux-kernel, Luís Henriques,
Baokun Li
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 <libaokun1@huawei.com>
Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
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;
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0
2022-08-12 10:53 [PATCH v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0 Luís Henriques
@ 2022-08-12 12:50 ` Baokun Li
2022-08-12 13:19 ` Luís Henriques
0 siblings, 1 reply; 4+ messages in thread
From: Baokun Li @ 2022-08-12 12:50 UTC (permalink / raw)
To: Luís Henriques, Theodore Ts'o, Andreas Dilger
Cc: wenqingliu0120, linux-ext4, linux-kernel, zhangyi (F), yebin10,
yukuai (C), Baokun Li
Hi Luís,
On 8/12/2022 6:53 PM, Luís Henriques wrote:
> 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 <libaokun1@huawei.com>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> 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)))) {
The parentheses are misplaced, and le16_to_cpu is not needed here.
> + 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0
2022-08-12 12:50 ` Baokun Li
@ 2022-08-12 13:19 ` Luís Henriques
2022-08-12 13:34 ` Baokun Li
0 siblings, 1 reply; 4+ messages in thread
From: Luís Henriques @ 2022-08-12 13:19 UTC (permalink / raw)
To: Baokun Li
Cc: Theodore Ts'o, Andreas Dilger, wenqingliu0120, linux-ext4,
linux-kernel, zhangyi (F), yebin10, yukuai (C)
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0
2022-08-12 13:19 ` Luís Henriques
@ 2022-08-12 13:34 ` Baokun Li
0 siblings, 0 replies; 4+ messages in thread
From: Baokun Li @ 2022-08-12 13:34 UTC (permalink / raw)
To: Luís Henriques
Cc: Theodore Ts'o, Andreas Dilger, wenqingliu0120, linux-ext4,
linux-kernel, zhangyi (F), yebin10, yukuai (C), Baokun Li
Hi Luís,
On 8/12/2022 9:19 PM, Luís Henriques wrote:
> 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)))) {
le16_to_cpu(eh->eh_depth > 0) It's the wrong position of the parentheses here.
>> 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))) {
> /* ... */
> }
Yeah, but it could be more streamlined here.
The earlier judgment has guaranteed "depth == eh->eh_depth"
> And once again, thanks for your review!
>
> Cheers,
--
With Best Regards,
Baokun Li
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-12 13:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-12 10:53 [PATCH v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0 Luís Henriques
2022-08-12 12:50 ` Baokun Li
2022-08-12 13:19 ` Luís Henriques
2022-08-12 13:34 ` Baokun Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox