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]

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

On Thu, 17 Oct 2013, Theodore Ts'o wrote:

> Date: Thu, 17 Oct 2013 10:40:44 -0400
> From: Theodore Ts'o <tytso@....edu>
> To: Lukáš Czerner <lczerner@...hat.com>
> Cc: Eryu Guan <guaneryu@...il.com>, linux-ext4@...r.kernel.org
> Subject: Re: [PATCH] ext4: don't cache out of order extents
> 
> On Thu, Oct 17, 2013 at 03:44:35PM +0200, Lukáš Czerner wrote:
> > 
> > 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.
> 
> Ah, I see what's going wrong.  __read_extent_tree_block() calls
> __ext4_ext_check() which is supposed to validate that extent tree
> block is valid.  The __ext4_ext_check() function calls
> ext4_valid_extent_entries() which is supposed to validate the
> individual entries in the extent.  However, it is not checking to make
> sure there are no overlapping extents.  We should add that check to
> ext4_valid_extent_entries(); that way, we will call ext4_error_inode()
> to mark the file system as corrupted.

I agree, since ext4_ext_check() should be really only used when
reading data from disk. That said, we might actually remove the
check from ext4_ext_precache() and ext4_ext_remove_space() because
it seems to be that the check has already been done in ext4_iget()
and it should be enough to check it once when reading from disk,
right ?

> 
> Eryu's patch, or something like it, will still be needed so that in
> the case of errors=countinue, we don't end up calling BUG_ON().

Hmm shouldn't we avoid that data in the case that it's corrupted
rather than using it ? It seems like this is what the code would do
anyway even with errors=continue when __ext4_ext_check() returns
error.

Thanks!
-Lukas

> 
> Thanks for finding this!
> 
> 						- Ted
>