linux-ext4 - Re: [PATCH 06/10] e2fsck: Handle s_inodes

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: <716AF6B2-47BF-418D-997C-4CC4BD431555@dilger.ca>
Date:   Sun, 3 Jun 2018 22:50:39 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Jan Kara <jack@...e.cz>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 06/10] e2fsck: Handle s_inodes_count corruption properly

On May 30, 2018, at 6:51 AM, Jan Kara <jack@...e.cz> wrote:
> 
> When s_inodes_count would overflow given number of groups and inodes per
> group, we cannot currently fix the breakage in e2fsck as that requires
> trimming number of groups or inodes per group which both means data &
> inode migration etc. So don't pretend we can fix it by just trimming the
> s_inodes_count value.
> 
> When s_inodes_count is just wrong but will not overflow, let's fix it.
> Also move this check before we use s_inodes_count to checking
> s_first_ino.
> 
> Signed-off-by: Jan Kara <jack@...e.cz>

Some comments inline.

> ---
> e2fsck/problem.c |  5 +++++
> e2fsck/problem.h |  2 ++
> e2fsck/super.c   | 27 ++++++++++++++++-----------
> 3 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index edc9d51fcf31..8de558edc5dd 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -184,6 +184,11 @@ static struct e2fsck_problem problem_table[] = {
> 	  N_("@i count in @S is %i, @s %j.\n"),
> 	  PROMPT_FIX, 0 },
> 
> +	/* Too many inodes in the filesystem */
> +	{ PR_0_INODE_COUNT_BIG,
> +	  N_("@S would have too many inodes (%N).\n"),
> +	  PROMPT_NONE, PR_AFTER_CODE, PR_0_SB_CORRUPT },
> +
> 	{ PR_0_HURD_CLEAR_FILETYPE,
> 	  N_("The Hurd does not support the filetype feature.\n"),
> 	  PROMPT_CLEAR, 0 },
> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index 482d111a0f2c..b6bca668bb1b 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -282,6 +282,8 @@ struct problem_context {
> /* Invalid quota inode number */
> #define PR_0_INVALID_QUOTA_INO			0x00004F
> 
> +/* Inode count in the superblock incorrect */
> +#define PR_0_INODE_COUNT_BIG			0x000050
> 
> /*
>  * Pass 1 errors
> diff --git a/e2fsck/super.c b/e2fsck/super.c
> index 9c0e0e7b35b4..00872b5ce5dc 100644
> --- a/e2fsck/super.c
> +++ b/e2fsck/super.c
> @@ -646,6 +646,22 @@ void check_super_block(e2fsck_t ctx)
> 	check_super_value(ctx, "desc_size",
> 			  sb->s_desc_size, MAX_CHECK | LOG2_CHECK, 0,
> 			  EXT2_MAX_DESC_SIZE);
> +
> +	should_be = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;

I don't think "blk64_t" is quite the right type here either, that is
intended for 64-bit block numbers, while you are computing a 64-bit
inode number...

> +	if (should_be > UINT_MAX) {
> +		pctx.num = should_be;
> +		fix_problem(ctx, PR_0_INODE_COUNT_BIG, &pctx);
> +		ctx->flags |= E2F_FLAG_ABORT;
> +		return;
> +	}

I think this should also handle the case hit by the original bug reporter,
"should_be == 2^32".  That would likely be the common case hit here, since
the number of inodes per group is typically a power of two value and
resizing the filesystem too much would hit exactly 2^32.  Something like:

	if (should_be == (unsigned long long)UINT_MAX + 1) {
		should_be = UINT_MAX;
	} else if (should_be > UINT_MAX) {
		:
		:

This matches the behaviour in mke2fs that converts 2^32 inodes to 2^32-1.
Otherwise, it is possible for a valid filesystem formatted by mke2fs with
2^32-1 inodes cannot be checked by e2fsck.

Also, in patch 01/10 it showed "~0U" for the maximum inode count is used
in ext2fs/initialize.c, but this is using "UINT_MAX".  While they are likely
the same, it would be good to be consistent.

Cheers, Andreas

> +	if (sb->s_inodes_count != should_be) {
> +		pctx.ino = sb->s_inodes_count;
> +		pctx.ino2 = should_be;
> +		if (fix_problem(ctx, PR_0_INODE_COUNT_WRONG, &pctx)) {
> +			sb->s_inodes_count = should_be;
> +			ext2fs_mark_super_dirty(fs);
> +		}
> +	}
> 	if (sb->s_rev_level > EXT2_GOOD_OLD_REV)
> 		check_super_value(ctx, "first_ino", sb->s_first_ino,
> 				  MIN_CHECK | MAX_CHECK,
> @@ -683,17 +699,6 @@ void check_super_block(e2fsck_t ctx)
> 		return;
> 	}
> 
> -	should_be = (blk64_t)sb->s_inodes_per_group * fs->group_desc_count;
> -	if (should_be > UINT_MAX)
> -		should_be = UINT_MAX;
> -	if (sb->s_inodes_count != should_be) {
> -		pctx.ino = sb->s_inodes_count;
> -		pctx.ino2 = should_be;
> -		if (fix_problem(ctx, PR_0_INODE_COUNT_WRONG, &pctx)) {
> -			sb->s_inodes_count = should_be;
> -			ext2fs_mark_super_dirty(fs);
> -		}
> -	}
> 	if (EXT2_INODE_SIZE(sb) > EXT2_GOOD_OLD_INODE_SIZE) {
> 		unsigned min =
> 			sizeof(((struct ext2_inode_large *) 0)->i_extra_isize) +
> --
> 2.13.6
> 


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)