linux-ext4 - Re: [PATCH] ext4: Fix checksum errors with indexed dirs

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: <BC1AA070-8C16-4399-B4D8-1E9F24D05D8D@dilger.ca>
Date:   Wed, 5 Feb 2020 11:04:23 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Jan Kara <jack@...e.cz>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH] ext4: Fix checksum errors with indexed dirs

On Feb 5, 2020, at 10:30 AM, Jan Kara <jack@...e.cz> wrote:
> 
> DIR_INDEX has been introduced as a compat ext4 feature. That means that
> even kernels / tools that don't understand the feature may modify the
> filesystem. This works because for kernels not understanding indexed dir
> format, internal htree nodes appear just as empty directory entries.
> Index dir aware kernels then check the htree structure is still
> consistent before using the data. This all worked reasonably well until
> metadata checksums were introduced. The problem is that these
> effectively made DIR_INDEX only ro-compatible because internal htree
> nodes store checksums in a different place than normal directory blocks.
> Thus any modification ignorant to DIR_INDEX (or just clearing
> EXT4_INDEX_FL from the inode) will effectively cause checksum mismatch
> and trigger kernel errors. So we have to be more careful when dealing
> with indexed directories on filesystems with checksumming enabled.

> 1) We just disallow loading and directory inodes with EXT4_INDEX_FL when

s/and/any/ ?

> DIR_INDEX is not enabled. This is harsh but it should be very rare (it
> means someone disabled DIR_INDEX on existing filesystem and didn't run
> e2fsck), e2fsck can fix the problem, and we don't want to answer the
> difficult question: "Should we rather corrupt the directory more or
> should we ignore that DIR_INDEX feature is not set?"

Wouldn't it be better to continue allowing the directory to be read, but
not modified?  Otherwise, essentially, metadata_csum is making the
filesystem _less_ robust rather than making it more robust.  We don't
_need_ the htree index to do a lookup in the directory.

> 2) When we find out htree structure is corrupted (but the filesystem and
> the directory should in support htrees), we continue just ignoring htree
> information for reading but we refuse to add new entries to the
> directory to avoid corrupting it more.
> 
> CC: stable@...r.kernel.org
> Fixes: dbe89444042a ("ext4: Calculate and verify checksums for htree nodes")
> Signed-off-by: Jan Kara <jack@...e.cz>
> ---
> fs/ext4/dir.c   | 14 ++++++++------
> fs/ext4/ext4.h  |  5 ++++-
> fs/ext4/inode.c | 13 +++++++++++++
> fs/ext4/namei.c |  7 +++++++
> 4 files changed, 32 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 629a25d999f0..d33135308c1b 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4615,6 +4615,19 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
> 		ret = -EFSCORRUPTED;
> 		goto bad_inode;
> 	}
> +	/*
> +	 * If dir_index is not enabled but there's dir with INDEX flag set,
> +	 * we'd normally treat htree data as empty space. But with metadata
> +	 * checksumming that corrupts checksums so forbid that.
> +	 */
> +	if (!ext4_has_feature_dir_index(sb) && ext4_has_metadata_csum(sb) &&
> +	    ext4_test_inode_flag(inode, EXT4_INODE_INDEX)) {
> +		ext4_error_inode(inode, function, line, 0,
> +				 "iget: Dir with htree data on filesystem "
> +				 "without dir_index feature.");

Kernel style suggests error strings should not be line wrapped at 80 columns.


Cheers, Andreas






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