linux-ext4 - Re: [PATCH] ext4: inherit encryption xattr before other xattrs

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: <2F4F7FA9-37C2-4FF1-9365-EF38D57FE7CA@dilger.ca>
Date:   Mon, 27 Feb 2017 13:28:28 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux-fscrypt@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH] ext4: inherit encryption xattr before other xattrs

On Feb 17, 2017, at 4:33 PM, Eric Biggers <ebiggers3@...il.com> wrote:
> 
> From: Eric Biggers <ebiggers@...gle.com>
> 
> When using both encryption and SELinux (or another feature that requires
> an xattr per file) on a filesystem with 256-byte inodes, each file's
> xattrs usually spill into an external xattr block.  Currently, the
> xattrs are inherited in the order ACL, security, then encryption.
> Therefore, if spillage occurs, the encryption xattr will always end up
> in the external block.  This is not ideal because the encryption xattrs
> contain a nonce, so they will always be unique and will prevent the
> external xattr blocks from being deduplicated.
> 
> To improve the situation, change the inheritance order to encryption,
> ACL, then security.  This gives the encryption xattr a better chance to
> be stored in-inode, allowing the other xattr(s) to be deduplicated.
> 
> Note that it may be better for userspace to format the filesystem with
> 512-byte inodes in this case.  However, it's not the default.
> 
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>

Reviewed-by: Andreas Dilger <adilger@...ger.ca>

Note that we've been using 512-byte xattrs for Lustre metadata servers
for ages.  We may want to consider enabling this by default when the
filesystem features are set (e.g. crypto, inline data, etc).

Having a general mechanism to bias xattrs to in-inode or external xattr
storage would be nice also, but I don't know any way to do this beyond
just having a list of xattr names and then prioritizing the ones that
go into the in-inode space.

Cheers, Andreas

> ---
> fs/ext4/ialloc.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index b14bae2598bc..0304e28c2014 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1096,6 +1096,17 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
> 	if (err)
> 		goto fail_drop;
> 
> +	/*
> +	 * Since the encryption xattr will always be unique, create it first so
> +	 * that it's less likely to end up in an external xattr block and
> +	 * prevent its deduplication.
> +	 */
> +	if (encrypt) {
> +		err = fscrypt_inherit_context(dir, inode, handle, true);
> +		if (err)
> +			goto fail_free_drop;
> +	}
> +
> 	err = ext4_init_acl(handle, inode, dir);
> 	if (err)
> 		goto fail_free_drop;
> @@ -1117,12 +1128,6 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
> 		ei->i_datasync_tid = handle->h_transaction->t_tid;
> 	}
> 
> -	if (encrypt) {
> -		err = fscrypt_inherit_context(dir, inode, handle, true);
> -		if (err)
> -			goto fail_free_drop;
> -	}
> -
> 	err = ext4_mark_inode_dirty(handle, inode);
> 	if (err) {
> 		ext4_std_error(sb, err);
> --
> 2.11.0.483.g087da7b7c-goog
> 


Cheers, Andreas






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