| Message ID | 1382275647-4616-1-git-send-email-guaneryu@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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
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); >
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);
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(+)