linux-ext4 - Re: [PATCH] ext4: don't cache out of order extents

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1310171534440.3212@localhost.localdomain>
Date:	Thu, 17 Oct 2013 15:44:35 +0200 (CEST)
From:	Lukáš Czerner <lczerner@...hat.com>
To:	Eryu Guan <guaneryu@...il.com>
cc:	linux-ext4@...r.kernel.org, "Theodore Ts'o" <tytso@....edu>
Subject: Re: [PATCH] ext4: don't cache out of order extents

On Thu, 17 Oct 2013, Eryu Guan wrote:

> Date: Thu, 17 Oct 2013 17:27:53 +0800
> From: Eryu Guan <guaneryu@...il.com>
> To: linux-ext4@...r.kernel.org
> Cc: Eryu Guan <guaneryu@...il.com>, Theodore Ts'o <tytso@....edu>
> Subject: [PATCH] ext4: don't cache out of order extents
> 
> 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().
> 
> 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.

So what will happen with the file system with this patch when
presented with such corruption ?

It seems to me that ext4_es_cache_extent() will happily skip this
extent because it will find that this particular offset is already
in the tree. Hence we'll have a gap in the status tree which really
should not be there and I suspect that something bad will happen.

I think that we should deal with this corruption immediately when we
spot it there, not just hide it.

Thanks!
-Lukas

> 
> Cc: "Theodore Ts'o" <tytso@....edu>
> Signed-off-by: Eryu Guan <guaneryu@...il.com>
> ---
>  fs/ext4/extents.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 54d52af..c9ebcb9 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -503,7 +503,7 @@ __read_extent_tree_block(const char *function, unsigned int line,
>  			ext4_lblk_t lblk = le32_to_cpu(ex->ee_block);
>  			int len = ext4_ext_get_actual_len(ex);
>  
> -			if (prev && (prev != lblk))
> +			if (prev && (prev < lblk))
>  				ext4_es_cache_extent(inode, prev,
>  						     lblk - prev, ~0,
>  						     EXTENT_STATUS_HOLE);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html