linux-ext4 - Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

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:	Tue, 23 Jul 2013 16:35:46 -0500
From:	Eric Sandeen <sandeen@...hat.com>
To:	Eric Whitney <enwlinux@...il.com>
CC:	linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH] e2fsprogs: Don't report uninit extents past EOF invalid

On 7/21/13 3:28 PM, Eric Whitney wrote:
> Commit d3f32c2db8 caused e2fsck misbehavior during xfstests runs.
> It reported that uninitialized extents created by fallocate() at
> the end of file with the FALLOC_FL_KEEP_SIZE flag were invalid.
> Because FALLOC_FL_KEEP_SIZE does not increase the file size when
> an extent is fallocated, an uninitialized extent can legally contain
> blocks past the end of file.
> 
> The information reported by ext2fs_extent_get() and used by the commit
> to determine legal extent ranges is limited by the value of i_size
> (determines end_blk in the root extent index), so block values greater
> than that containing i_size were reported as invalid.
> 
> To fix this, filter out possible invalid extent candidates if they are
> uninitialized and extend past the block containing the end of file.

The bummer about this approach is that it won't check for overlap
of any extents past EOF which are unwritten (of which there could be many).

It's probably worth merging now just to take care of the false positives,
but I think it's leaving some potential corruptions undetected.

(corruptions which I'm not yet sure how to detect ...)

-Eric

> Signed-off-by: Eric Whitney <enwlinux@...il.com>
> ---
>  e2fsck/pass1.c      |    4 +++-
>  lib/ext2fs/ext2fs.h |    1 +
>  lib/ext2fs/extent.c |    1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index ba6025b..b84b0d0 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -1892,7 +1892,9 @@ static void scan_extent_node(e2fsck_t ctx, struct problem_context *pctx,
>  			problem = PR_1_EXTENT_BAD_START_BLK;
>  		else if (extent.e_lblk < start_block)
>  			problem = PR_1_OUT_OF_ORDER_EXTENTS;
> -		else if (end_block && last_lblk > end_block)
> +		else if ((end_block && last_lblk > end_block) &&
> +			 (!(extent.e_flags & EXT2_EXTENT_FLAGS_UNINIT &&
> +			    last_lblk > info.eof_blk - 1)))
>  			problem = PR_1_EXTENT_END_OUT_OF_BOUNDS;
>  		else if (is_leaf && extent.e_len == 0)
>  			problem = PR_1_EXTENT_LENGTH_ZERO;
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 311ceda..85f2ac8 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -409,6 +409,7 @@ struct ext2_extent_info {
>  	int		bytes_avail;
>  	blk64_t		max_lblk;
>  	blk64_t		max_pblk;
> +	blk64_t         eof_blk;
>  	__u32		max_len;
>  	__u32		max_uninit_len;
>  };
> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
> index 65bb099..de54319 100644
> --- a/lib/ext2fs/extent.c
> +++ b/lib/ext2fs/extent.c
> @@ -1572,6 +1572,7 @@ errcode_t ext2fs_extent_get_info(ext2_extent_handle_t handle,
>  	info->max_depth = handle->max_depth;
>  	info->max_lblk = ((__u64) 1 << 32) - 1;
>  	info->max_pblk = ((__u64) 1 << 48) - 1;
> +	info->eof_blk = handle->path[0].end_blk;
>  	info->max_len = (1UL << 15);
>  	info->max_uninit_len = (1UL << 15) - 1;
>  
> 

--
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