Linux-f2fs-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Daniel Rosenberg <drosen@google.com>
Cc: Theodore Ts'o <tytso@mit.edu>, Jonathan Corbet <corbet@lwn.net>,
	kernel-team@android.com, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fscrypt@vger.kernel.org,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org, Jaegeuk Kim <jaegeuk@kernel.org>,
	linux-ext4@vger.kernel.org,
	Gabriel Krisman Bertazi <krisman@collabora.com>
Subject: Re: [f2fs-dev] [PATCH 1/8] fscrypt: Add siphash and hash key for policy v2
Date: Tue, 3 Dec 2019 15:25:42 -0800
Message-ID: <20191203232542.GB727@sol.localdomain> (raw)
In-Reply-To: <20191203051049.44573-2-drosen@google.com>

On Mon, Dec 02, 2019 at 09:10:42PM -0800, Daniel Rosenberg wrote:
> When using casefolding along with encryption, we need to use a
> cryptographic hash to allow fast filesystem operations while not knowing
> the case of the name stored on disk while not revealing extra
> information about the name if the key is not present.

This sentence is hard to parse.  Can you make it any clearer?

> 
> When a v2 policy is used on a directory, we derive a key for use with
> siphash.
> 
> Signed-off-by: Daniel Rosenberg <drosen@google.com>
> ---
>  fs/crypto/fname.c           | 22 ++++++++++++++++++++++
>  fs/crypto/fscrypt_private.h |  9 +++++++++
>  fs/crypto/keysetup.c        | 29 ++++++++++++++++++++---------
>  include/linux/fscrypt.h     |  8 ++++++++
>  4 files changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 3da3707c10e3..b33f03b9f892 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include <linux/scatterlist.h>
> +#include <linux/siphash.h>
>  #include <crypto/skcipher.h>
>  #include "fscrypt_private.h"
>  
> @@ -400,3 +401,24 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
>  	return ret;
>  }
>  EXPORT_SYMBOL(fscrypt_setup_filename);
> +
> +/**
> + * fscrypt_fname_siphash() - Calculate the siphash for a file name
> + * @dir: the parent directory
> + * @name: the name of the file to get the siphash of
> + *
> + * Given a user-provided filename @name, this function calculates the siphash of
> + * that name using the hash key stored with the directory's policy.

I suggest writing "using the directory's hash key" instead of "using the hash
key stored with the directory's policy", since the latter might be misunderstood
as meaning that the hash key is stored on-disk.

Also it would be helpful to document the assumptions:

	The directory must use a v2 encryption policy, and its key must be available.

> + *
> + *
> + * Return: the siphash of @name using the hash key of @dir
> + */
> +u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name)
> +{
> +	struct fscrypt_info *ci = dir->i_crypt_info;
> +
> +	WARN_ON(!ci || !ci->ci_hash_key_initialized);
> +
> +	return siphash(name->name, name->len, &ci->ci_hash_key);
> +}
> +EXPORT_SYMBOL(fscrypt_fname_siphash);

The !ci part of the WARN_ON is pointless because if it ever triggers, there will
be a NULL dereference afterwards anyway.  I suggest changing it to just:

	WARN_ON(!ci->ci_hash_key_initialized);

> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 130b50e5a011..f0dfef9921de 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -12,6 +12,7 @@
>  #define _FSCRYPT_PRIVATE_H
>  
>  #include <linux/fscrypt.h>
> +#include <linux/siphash.h>
>  #include <crypto/hash.h>
>  
>  #define CONST_STRLEN(str)	(sizeof(str) - 1)
> @@ -194,6 +195,13 @@ struct fscrypt_info {
>  	 */
>  	struct fscrypt_direct_key *ci_direct_key;
>  
> +	/*
> +	 * With v2 policies, this can be used with siphash
> +	 * When the key has been set, ci_hash_key_initialized is set to true
> +	 */
> +	siphash_key_t ci_hash_key;
> +	bool ci_hash_key_initialized;
> +
>  	/* The encryption policy used by this inode */
>  	union fscrypt_policy ci_policy;
>  
> @@ -286,6 +294,7 @@ extern int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
>  #define HKDF_CONTEXT_PER_FILE_KEY	2
>  #define HKDF_CONTEXT_DIRECT_KEY		3
>  #define HKDF_CONTEXT_IV_INO_LBLK_64_KEY	4
> +#define HKDF_CONTEXT_FNAME_HASH_KEY     5
>  
>  extern int fscrypt_hkdf_expand(struct fscrypt_hkdf *hkdf, u8 context,
>  			       const u8 *info, unsigned int infolen,
> diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c
> index f577bb6613f9..e6c7ec04cd25 100644
> --- a/fs/crypto/keysetup.c
> +++ b/fs/crypto/keysetup.c
> @@ -192,7 +192,7 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
>  				     ci->ci_mode->friendly_name);
>  			return -EINVAL;
>  		}
> -		return setup_per_mode_key(ci, mk, mk->mk_direct_tfms,
> +		err = setup_per_mode_key(ci, mk, mk->mk_direct_tfms,
>  					  HKDF_CONTEXT_DIRECT_KEY, false);
>  	} else if (ci->ci_policy.v2.flags &
>  		   FSCRYPT_POLICY_FLAG_IV_INO_LBLK_64) {
> @@ -202,20 +202,31 @@ static int fscrypt_setup_v2_file_key(struct fscrypt_info *ci,
>  		 * the IVs.  This format is optimized for use with inline
>  		 * encryption hardware compliant with the UFS or eMMC standards.
>  		 */
> -		return setup_per_mode_key(ci, mk, mk->mk_iv_ino_lblk_64_tfms,
> +		err = setup_per_mode_key(ci, mk, mk->mk_iv_ino_lblk_64_tfms,
>  					  HKDF_CONTEXT_IV_INO_LBLK_64_KEY,
>  					  true);
> -	}
> -
> -	err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
> +	} else {
> +		err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
>  				  HKDF_CONTEXT_PER_FILE_KEY,
>  				  ci->ci_nonce, FS_KEY_DERIVATION_NONCE_SIZE,
>  				  derived_key, ci->ci_mode->keysize);

Nit: keep continuation lines aligned when they don't exceed 80 characters:

		err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
					  HKDF_CONTEXT_PER_FILE_KEY,
					  ci->ci_nonce,
					  FS_KEY_DERIVATION_NONCE_SIZE,
					  derived_key, ci->ci_mode->keysize);

> -	if (err)
> -		return err;
> +		if (err)
> +			return err;
> +
> +		err = fscrypt_set_derived_key(ci, derived_key);
> +		memzero_explicit(derived_key, ci->ci_mode->keysize);
> +		if (err)
> +			return err;

This 'if (err)' check is in the wrong place.  It needs to be below the brace
below, so that it also checks the error from setup_per_mode_key().

> +	}
>  
> -	err = fscrypt_set_derived_key(ci, derived_key);
> -	memzero_explicit(derived_key, ci->ci_mode->keysize);
> +	if (S_ISDIR(ci->ci_inode->i_mode)) {
> +		err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
> +			  HKDF_CONTEXT_FNAME_HASH_KEY,
> +			  ci->ci_nonce, FS_KEY_DERIVATION_NONCE_SIZE,
> +			  (u8 *)&ci->ci_hash_key, sizeof(ci->ci_hash_key));

Nit: keep continuation lines aligned when they don't exceed 80 characters:

		err = fscrypt_hkdf_expand(&mk->mk_secret.hkdf,
					  HKDF_CONTEXT_FNAME_HASH_KEY,
					  ci->ci_nonce,
					  FS_KEY_DERIVATION_NONCE_SIZE,
					  (u8 *)&ci->ci_hash_key,
					  sizeof(ci->ci_hash_key));

> +		if (!err)
> +			ci->ci_hash_key_initialized = true;
> +	}
>  	return err;

Nit: an early return on error would be better here
(consistent with the code above):

                if (err)
                        return err;
                ci->ci_hash_key_initialized = true;
        }
        return 0;

> +extern u64 fscrypt_fname_siphash(const struct inode *dir,
> +					const struct qstr *name);

Nit: align the continuation line:

extern u64 fscrypt_fname_siphash(const struct inode *dir,
				 const struct qstr *name);

>  
>  #define FSCRYPT_FNAME_MAX_UNDIGESTED_SIZE	32
>  
> @@ -446,6 +448,12 @@ static inline int fscrypt_fname_disk_to_usr(struct inode *inode,
>  	return -EOPNOTSUPP;
>  }
>  
> +static inline u64 fscrypt_fname_siphash(const struct inode *inode,

In the other places the first parameter is called 'dir', not 'inode'.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-03  5:10 [f2fs-dev] [PATCH 0/8] Support for Casefolding and Encryption Daniel Rosenberg via Linux-f2fs-devel
2019-12-03  5:10 ` [f2fs-dev] [PATCH 1/8] fscrypt: Add siphash and hash key for policy v2 Daniel Rosenberg via Linux-f2fs-devel
2019-12-03 23:25   ` Eric Biggers [this message]
2019-12-03  5:10 ` [f2fs-dev] [PATCH 2/8] fscrypt: Don't allow v1 policies with casefolding Daniel Rosenberg via Linux-f2fs-devel
2019-12-03 23:37   ` Eric Biggers
2019-12-03  5:10 ` [f2fs-dev] [PATCH 3/8] fscrypt: Change format of no-key token Daniel Rosenberg via Linux-f2fs-devel
2019-12-04  0:09   ` Eric Biggers
2019-12-03  5:10 ` [f2fs-dev] [PATCH 4/8] vfs: Fold casefolding into vfs Daniel Rosenberg via Linux-f2fs-devel
2019-12-03  7:41   ` Gao Xiang
2019-12-03 19:42     ` Gabriel Krisman Bertazi
2019-12-03 20:34       ` Eric Biggers
2019-12-03 21:21         ` Gabriel Krisman Bertazi
2019-12-04  0:32           ` Eric Biggers
2019-12-03 19:31   ` Gabriel Krisman Bertazi
2019-12-03  5:10 ` [f2fs-dev] [PATCH 5/8] f2fs: Handle casefolding with Encryption Daniel Rosenberg via Linux-f2fs-devel
2019-12-05  1:17   ` kbuild test robot
2019-12-03  5:10 ` [f2fs-dev] [PATCH 6/8] ext4: Use struct super_block's casefold data Daniel Rosenberg via Linux-f2fs-devel
2019-12-03 19:44   ` Gabriel Krisman Bertazi
2019-12-03  5:10 ` [f2fs-dev] [PATCH 7/8] ext4: Hande casefolding with encryption Daniel Rosenberg via Linux-f2fs-devel
2019-12-03  5:10 ` [f2fs-dev] [PATCH 8/8] ext4: Optimize match for casefolded encrypted dirs Daniel Rosenberg via Linux-f2fs-devel

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191203232542.GB727@sol.localdomain \
    --to=ebiggers@kernel.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=corbet@lwn.net \
    --cc=drosen@google.com \
    --cc=jaegeuk@kernel.org \
    --cc=kernel-team@android.com \
    --cc=krisman@collabora.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-f2fs-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-f2fs-devel/0 linux-f2fs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-f2fs-devel linux-f2fs-devel/ https://lore.kernel.org/linux-f2fs-devel \
		linux-f2fs-devel@lists.sourceforge.net
	public-inbox-index linux-f2fs-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.linux-f2fs-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git