[PATCH] ext4: Make the extent validity check more paranoid

[PATCH] ext4: Make the extent validity check more paranoid

[Theodore Ts'o]

Instead of just checking that the extent block number is greater or
equal than s_first_data_block, make sure it it is not pointing into
the block group descriptors, since that is clearly wrong.  This helps
prevent filesystem from getting very badly corrupted in case an extent
block is corrupted.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/extents.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 6132353..c28ffe2 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -326,11 +326,14 @@ ext4_ext_max_entries(struct inode *inode, int depth)
 
 static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext)
 {
-	ext4_fsblk_t block = ext_pblock(ext);
+	ext4_fsblk_t block = ext_pblock(ext), valid_block;
 	int len = ext4_ext_get_actual_len(ext);
 	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
-	if (unlikely(block < le32_to_cpu(es->s_first_data_block) ||
-			((block + len) > ext4_blocks_count(es))))
+
+	valid_block = le32_to_cpu(es->s_first_data_block) +
+		EXT4_SB(inode->i_sb)->s_gdb_count;
+	if (unlikely(block <= valid_block ||
+		     ((block + len) > ext4_blocks_count(es))))
 		return 0;
 	else
 		return 1;
@@ -339,10 +342,13 @@ static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext)
 static int ext4_valid_extent_idx(struct inode *inode,
 				struct ext4_extent_idx *ext_idx)
 {
-	ext4_fsblk_t block = idx_pblock(ext_idx);
+	ext4_fsblk_t block = idx_pblock(ext_idx), valid_block;
 	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
-	if (unlikely(block < le32_to_cpu(es->s_first_data_block) ||
-			(block >= ext4_blocks_count(es))))
+
+	valid_block = le32_to_cpu(es->s_first_data_block) +
+		EXT4_SB(inode->i_sb)->s_gdb_count;
+	if (unlikely(block <= valid_block ||
+		     (block >= ext4_blocks_count(es))))
 		return 0;
 	else
 		return 1;
-- 
1.5.6.3

Re: [PATCH] ext4: Make the extent validity check more paranoid

[Eric Sandeen]

Theodore Ts'o wrote:
> Instead of just checking that the extent block number is greater or
> equal than s_first_data_block, make sure it it is not pointing into
> the block group descriptors, since that is clearly wrong.  This helps
> prevent filesystem from getting very badly corrupted in case an extent
> block is corrupted.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Good idea.  Maybe we can get our friends with the corrupted fs to run
with these validation patches... I can get this into rawhide at least.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/ext4/extents.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 6132353..c28ffe2 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -326,11 +326,14 @@ ext4_ext_max_entries(struct inode *inode, int depth)
>  
>  static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext)
>  {
> -	ext4_fsblk_t block = ext_pblock(ext);
> +	ext4_fsblk_t block = ext_pblock(ext), valid_block;
>  	int len = ext4_ext_get_actual_len(ext);
>  	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
> -	if (unlikely(block < le32_to_cpu(es->s_first_data_block) ||
> -			((block + len) > ext4_blocks_count(es))))
> +
> +	valid_block = le32_to_cpu(es->s_first_data_block) +
> +		EXT4_SB(inode->i_sb)->s_gdb_count;
> +	if (unlikely(block <= valid_block ||
> +		     ((block + len) > ext4_blocks_count(es))))
>  		return 0;
>  	else
>  		return 1;
> @@ -339,10 +342,13 @@ static int ext4_valid_extent(struct inode *inode, struct ext4_extent *ext)
>  static int ext4_valid_extent_idx(struct inode *inode,
>  				struct ext4_extent_idx *ext_idx)
>  {
> -	ext4_fsblk_t block = idx_pblock(ext_idx);
> +	ext4_fsblk_t block = idx_pblock(ext_idx), valid_block;
>  	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
> -	if (unlikely(block < le32_to_cpu(es->s_first_data_block) ||
> -			(block >= ext4_blocks_count(es))))
> +
> +	valid_block = le32_to_cpu(es->s_first_data_block) +
> +		EXT4_SB(inode->i_sb)->s_gdb_count;
> +	if (unlikely(block <= valid_block ||
> +		     (block >= ext4_blocks_count(es))))
>  		return 0;
>  	else
>  		return 1;

Re: [PATCH] ext4: Make the extent validity check more paranoid

[Theodore Tso]

On Wed, Apr 22, 2009 at 09:04:30PM -0500, Eric Sandeen wrote:
> Theodore Ts'o wrote:
> > Instead of just checking that the extent block number is greater or
> > equal than s_first_data_block, make sure it it is not pointing into
> > the block group descriptors, since that is clearly wrong.  This helps
> > prevent filesystem from getting very badly corrupted in case an extent
> > block is corrupted.
> > 
> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> Good idea.  Maybe we can get our friends with the corrupted fs to run
> with these validation patches... I can get this into rawhide at least.

Yeah, unfortunately this patch requires some other patches that went
in during the 2.6.30 merge window, so some extra back-porting would be
needed for our friends running a 2.6.29.1 Fedora kernel.

I'm still trying figure out what's the best way to add the right kind
of checking for 2.6.29 based kernels.  Looking at the dump files which
Kevin Shanahan, it doesn't look like they are coming from interior
nodes of extent trees.  Some kind of kludge patch into the block
device layer might be the most fool-proof way to do things.

       	     	      	       		  - Ted

Re: [PATCH] ext4: Make the extent validity check more paranoid

[Eric Sandeen]

Theodore Tso wrote:
> On Wed, Apr 22, 2009 at 09:04:30PM -0500, Eric Sandeen wrote:
>> Theodore Ts'o wrote:
>>> Instead of just checking that the extent block number is greater or
>>> equal than s_first_data_block, make sure it it is not pointing into
>>> the block group descriptors, since that is clearly wrong.  This helps
>>> prevent filesystem from getting very badly corrupted in case an extent
>>> block is corrupted.
>>>
>>> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>> Good idea.  Maybe we can get our friends with the corrupted fs to run
>> with these validation patches... I can get this into rawhide at least.
> 
> Yeah, unfortunately this patch requires some other patches that went
> in during the 2.6.30 merge window, so some extra back-porting would be
> needed for our friends running a 2.6.29.1 Fedora kernel.

Right, but rawhide has .30-etc, so a test on that might be helpful.

-Eric