Patchwork [v2] ext4: check for overlapping extents in ext4_valid_extent_entries()

login
register
mail settings
Submitter Eryu Guan
Date Oct. 20, 2013, 1:27 p.m.
Message ID <1382275647-4616-1-git-send-email-guaneryu@gmail.com>
Download mbox | patch
Permalink /patch/284972/
State Superseded
Headers show

Comments

Eryu Guan - Oct. 20, 2013, 1:27 p.m.
A corrupted ext4 may have out of order leaf extents, i.e.

extent: lblk 0--1023, len 1024, pblk 9217, flags: LEAF UNINIT
extent: lblk 1000--2047, len 1024, pblk 10241, flags: LEAF UNINIT
             ^^^^ overlap with previous extent

Reading such extent could hit BUG_ON() in ext4_es_cache_extent().

	BUG_ON(end < lblk);

The problem is that __read_extent_tree_block() tries to cache holes as
well but assumes 'lblk' is greater than 'prev' and passes underflowed
length to ext4_es_cache_extent(). Fix it by checking for overlapping
extents in ext4_valid_extent_entries().

I hit this when fuzz testing ext4, and am able to reproduce it by
modifying the on-disk extent by hand.

Ran xfstests on patched ext4 and no regression.

Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Lukáš Czerner <lczerner@redhat.com>
Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
Hi,

My second try to find and report the corruption instead of hiding it,
how about this one?

Thanks!

Eryu

 fs/ext4/extents.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
Eryu Guan - Oct. 21, 2013, 11:59 a.m.
On Sun, Oct 20, 2013 at 09:27:27PM +0800, Eryu Guan wrote:
> A corrupted ext4 may have out of order leaf extents, i.e.
> 
> extent: lblk 0--1023, len 1024, pblk 9217, flags: LEAF UNINIT
> extent: lblk 1000--2047, len 1024, pblk 10241, flags: LEAF UNINIT
>              ^^^^ overlap with previous extent
> 
> Reading such extent could hit BUG_ON() in ext4_es_cache_extent().
> 
> 	BUG_ON(end < lblk);
> 
> The problem is that __read_extent_tree_block() tries to cache holes as
> well but assumes 'lblk' is greater than 'prev' and passes underflowed
> length to ext4_es_cache_extent(). Fix it by checking for overlapping
> extents in ext4_valid_extent_entries().
> 
> I hit this when fuzz testing ext4, and am able to reproduce it by
> modifying the on-disk extent by hand.
> 
> Ran xfstests on patched ext4 and no regression.
> 
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Lukáš Czerner <lczerner@redhat.com>
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>

Self NAK, I found problems in tests, will think about it more.

Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan - Oct. 21, 2013, 12:47 p.m.
On Mon, Oct 21, 2013 at 07:59:54PM +0800, Eryu Guan wrote:
> On Sun, Oct 20, 2013 at 09:27:27PM +0800, Eryu Guan wrote:
> > A corrupted ext4 may have out of order leaf extents, i.e.
> > 
> > extent: lblk 0--1023, len 1024, pblk 9217, flags: LEAF UNINIT
> > extent: lblk 1000--2047, len 1024, pblk 10241, flags: LEAF UNINIT
> >              ^^^^ overlap with previous extent
> > 
> > Reading such extent could hit BUG_ON() in ext4_es_cache_extent().
> > 
> > 	BUG_ON(end < lblk);
> > 
> > The problem is that __read_extent_tree_block() tries to cache holes as
> > well but assumes 'lblk' is greater than 'prev' and passes underflowed
> > length to ext4_es_cache_extent(). Fix it by checking for overlapping
> > extents in ext4_valid_extent_entries().
> > 
> > I hit this when fuzz testing ext4, and am able to reproduce it by
> > modifying the on-disk extent by hand.
> > 
> > Ran xfstests on patched ext4 and no regression.
> > 
> > Cc: "Theodore Ts'o" <tytso@mit.edu>
> > Cc: Lukáš Czerner <lczerner@redhat.com>
> > Signed-off-by: Eryu Guan <guaneryu@gmail.com>
> 
> Self NAK, I found problems in tests, will think about it more.

Ah, the error messages in dmesg I've seen are from previous tests, so
there're no problems. Withdraw the self NAK.

Sorry for the noise..

Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - Oct. 21, 2013, 4:06 p.m.
On Sun, 20 Oct 2013, Eryu Guan wrote:

> Date: Sun, 20 Oct 2013 21:27:27 +0800
> From: Eryu Guan <guaneryu@gmail.com>
> To: linux-ext4@vger.kernel.org
> Cc: Eryu Guan <guaneryu@gmail.com>, Theodore Ts'o <tytso@mit.edu>,
>     Lukáš Czerner <lczerner@redhat.com>
> Subject: [PATCH v2] ext4: check for overlapping extents in
>     ext4_valid_extent_entries()
> 
> A corrupted ext4 may have out of order leaf extents, i.e.
> 
> extent: lblk 0--1023, len 1024, pblk 9217, flags: LEAF UNINIT
> extent: lblk 1000--2047, len 1024, pblk 10241, flags: LEAF UNINIT
>              ^^^^ overlap with previous extent
> 
> Reading such extent could hit BUG_ON() in ext4_es_cache_extent().
> 
> 	BUG_ON(end < lblk);
> 
> The problem is that __read_extent_tree_block() tries to cache holes as
> well but assumes 'lblk' is greater than 'prev' and passes underflowed
> length to ext4_es_cache_extent(). Fix it by checking for overlapping
> extents in ext4_valid_extent_entries().
> 
> I hit this when fuzz testing ext4, and am able to reproduce it by
> modifying the on-disk extent by hand.
> 
> Ran xfstests on patched ext4 and no regression.

Looks ok, but I have some nitpicks bellow :)

> 
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Lukáš Czerner <lczerner@redhat.com>
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>
> ---
> Hi,
> 
> My second try to find and report the corruption instead of hiding it,
> how about this one?
> 
> Thanks!
> 
> Eryu
> 
>  fs/ext4/extents.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index c9ebcb9..855b11d 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -387,11 +387,21 @@ static int ext4_valid_extent_entries(struct inode *inode,
>  	if (depth == 0) {
>  		/* leaf entries */
>  		struct ext4_extent *ext = EXT_FIRST_EXTENT(eh);
> +		ext4_lblk_t block = 0;
> +		ext4_lblk_t prev = 0;
> +		int len = 0;
>  		while (entries) {
>  			if (!ext4_valid_extent(inode, ext))
>  				return 0;
> +
> +			/* Check for overlapping extents */
> +			block = le32_to_cpu(ext->ee_block);
> +			len = ext4_ext_get_actual_len(ext);
> +			if ((block <= prev) && prev)

Both ext4_valid_extent() and ext4_valid_extent_idx() are setting
s_last_error_block in the case of error. Maybe we should to the same
here ? Note that the block saved in that variable is physical, not
logical.

Also I am curious what happens when one of the extents is corrupted
in such a way that it crosses the 16TB boundary ? In this case the
check would not recognise that since prev will underflow, but maybe
something else catches that ?

Thanks!
-Lukas

> +				return 0;
>  			ext++;
>  			entries--;
> +			prev = block + len - 1;
>  		}
>  	} else {
>  		struct ext4_extent_idx *ext_idx = EXT_FIRST_INDEX(eh);
>

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index c9ebcb9..855b11d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -387,11 +387,21 @@  static int ext4_valid_extent_entries(struct inode *inode,
 	if (depth == 0) {
 		/* leaf entries */
 		struct ext4_extent *ext = EXT_FIRST_EXTENT(eh);
+		ext4_lblk_t block = 0;
+		ext4_lblk_t prev = 0;
+		int len = 0;
 		while (entries) {
 			if (!ext4_valid_extent(inode, ext))
 				return 0;
+
+			/* Check for overlapping extents */
+			block = le32_to_cpu(ext->ee_block);
+			len = ext4_ext_get_actual_len(ext);
+			if ((block <= prev) && prev)
+				return 0;
 			ext++;
 			entries--;
+			prev = block + len - 1;
 		}
 	} else {
 		struct ext4_extent_idx *ext_idx = EXT_FIRST_INDEX(eh);