diff mbox series

[v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0

Message ID 20220812105347.2251-1-lhenriques@suse.de
State Superseded
Headers show
Series [v2] ext4: fix bug in extents parsing when eh_entries == 0 and eh_depth > 0 | expand

Commit Message

Luis Henriques Aug. 12, 2022, 10:53 a.m. UTC 
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

Comments

Baokun Li Aug. 12, 2022, 12:50 p.m. UTC | #1 
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;
> .
Luis Henriques Aug. 12, 2022, 1:19 p.m. UTC | #2 
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,
Baokun Li Aug. 12, 2022, 1:34 p.m. UTC | #3 
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,
diff mbox series

Patch

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;