linux-ext4 - Re: [PATCH 03/12] e2fsck: ea

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:   Mon, 26 Jun 2017 15:21:50 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Tahsin Erdogan <tahsin@...gle.com>
Cc:     "Darrick J . Wong" <darrick.wong@...cle.com>,
        Theodore Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 03/12] e2fsck: ea_inode hash validation

On Jun 26, 2017, at 7:43 AM, Tahsin Erdogan <tahsin@...gle.com> wrote:
> 
> In original implementation of ea_inode feature, each xattr inode had
> a single parent. Child inode tracked the parent by storing its inode
> number in i_mtime field. Also, child's i_generation matched parent's
> i_generation.
> 
> With deduplication support, xattr inodes can now be shared so a single
> backpointer is not sufficient to achieve strong binding. This is now
> replaced by hash validation.
> 
> Signed-off-by: Tahsin Erdogan <tahsin@...gle.com>
> ---
> e2fsck/pass1.c        | 91 +++++++++++++++++++++++++++++++++------------------
> e2fsck/problem.c      |  5 ---
> e2fsck/problem.h      |  7 ++--
> lib/ext2fs/ext2fs.h   |  3 ++
> lib/ext2fs/ext_attr.c | 67 +++++++++++++++++++++++++++++++++++--
> 5 files changed, 128 insertions(+), 45 deletions(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 1c68af8990b4..32152f3ec926 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2439,6 +2466,16 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
> 						pctx))
> 					goto clear_extattr;
> 			}
> +
> +			hash = ext2fs_ext_attr_hash_entry(entry, block_buf +
> +							  entry->e_value_offs);
> +
> +			if (entry->e_hash != hash) {
> +				pctx->num = entry->e_hash;
> +				if (fix_problem(ctx, PR_1_ATTR_HASH, pctx))
> +					goto clear_extattr;

Shouldn't this be "if (!fix_problem(...))" ?


> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index dad608c94fd0..8e07c10895c1 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -678,15 +678,12 @@ struct problem_context {
> /* Inode has illegal EA value inode */
> #define PR_1_ATTR_VALUE_EA_INODE		0x010083
> 
> -/* Invalid backpointer from EA inode to parent inode */
> -#define PR_1_ATTR_INVAL_EA_INODE		0x010084
> -
> /* Parent inode has invalid EA entry. EA inode does not have
>  * EXT4_EA_INODE_FL flag. Delete EA entry? */
> -#define PR_1_ATTR_NO_EA_INODE_FL		0x010085
> +#define PR_1_ATTR_NO_EA_INODE_FL		0x010084
> 
> /* EA inode for parent inode does not have EXT4_EA_INODE_FL flag */
> -#define PR_1_ATTR_SET_EA_INODE_FL		0x010086
> +#define PR_1_ATTR_SET_EA_INODE_FL		0x010085

It would also be OK to keep the old values, and just skip 0x10084.

> diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
> index 14d05c7e57fb..d48ab5c55270 100644
> --- a/lib/ext2fs/ext_attr.c
> +++ b/lib/ext2fs/ext_attr.c
> @@ -914,9 +955,29 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
> 			void *data = (entry->e_value_inum != 0) ?
> 					0 : value_start + entry->e_value_offs;
> 
> -			hash = ext2fs_ext_attr_hash_entry(entry, data);
> -			if (entry->e_hash != hash)
> -				return EXT2_ET_BAD_EA_HASH;
> +			err = ext2fs_ext_attr_hash_entry2(handle->fs, entry,
> +							  data, &hash);
> +			if (err)
> +				return err;
> +			if (entry->e_hash != hash) {
> +				struct ext2_inode parent, child;
> +
> +				/* Check whether this is an old Lustre-style
> +				 * ea_inode reference.
> +				 */
> +				err = ext2fs_read_inode(handle->fs, handle->ino,
> +							&parent);

Rather than reading the inode again here, it would make more sense to pass the
parent inode from the caller.

> +				if (err)
> +					return err;
> +				err = ext2fs_read_inode(handle->fs,
> +							entry->e_value_inum,
> +							&child);
> +				if (err)
> +					return err;
> +				if (child.i_mtime != handle->ino ||
> +				    child.i_generation != parent.i_generation)
> +					return EXT2_ET_BAD_EA_HASH;
> +			}
> 		}
> 
> 		x++;
> --
> 2.13.1.611.g7e3b11ae1-goog
> 


Cheers, Andreas






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