Re: [PATCH 5/6] fscrypt: cache the HMAC transform for each master key
On Mon, Jul 17, 2017 at 10:45:51AM -0700, Michael Halcrow wrote: > > +/* > > + * Get the fscrypt_master_key identified by the specified v2+ encryption > > + * context, or create it if not found. > > + * > > + * Returns the fscrypt_master_key with a reference taken, or an ERR_PTR(). > > + */ > > +static struct fscrypt_master_key * > > +find_or_create_master_key(const struct inode *inode, > > + const struct fscrypt_context *ctx, > > + unsigned int min_keysize) > > +{ > > + struct fscrypt_master_key *master_key; > > + > > + if (WARN_ON(ctx->version < FSCRYPT_CONTEXT_V2)) > > + return ERR_PTR(-EINVAL); > > Isn't this a programming error? If so, consider either BUG_ON() or > omit the check. > Yes, but BUG_ON() is discouraged in cases where there is a straightforward way to recover from the error. checkpatch actually warns about using it these days. Eric
Re: [PATCH 5/6] fscrypt: cache the HMAC transform for each master key
On Wed, Jul 12, 2017 at 02:00:34PM -0700, Eric Biggers wrote: > From: Eric Biggers> > Now that we have a key_hash field which securely identifies a master key > payload, introduce a cache of the HMAC transforms for the master keys > currently in use for inodes using v2+ encryption policies. The entries > in this cache are called 'struct fscrypt_master_key' and are identified > by key_hash. The cache is per-superblock. (It could be global, but > making it per-superblock should reduce the lock contention a bit, and we > may need to keep track of keys on a per-superblock basis for other > reasons later, e.g. to implement an ioctl for evicting keys.) > > This results in a large efficiency gain: we now only have to allocate > and key an "hmac(sha512)" transformation, execute HKDF-Extract, and > compute key_hash once per master key rather than once per inode. Note > that this optimization can't easily be applied to the original AES-based > KDF because that uses a different AES key for each KDF execution. In > practice, this difference makes the HKDF per-inode encryption key > derivation performance comparable to or even faster than the old KDF, > which typically spends more time allocating an "ecb(aes)" transformation > from the crypto API than doing actual crypto work. > > Note that it would have been possible to make the mapping be from > raw_key => fscrypt_master_key (where raw_key denotes the actual bytes of > the master key) rather than from key_hash => fscrypt_master_key. > However, an advantage of doing lookups by key_hash is that it replaces > the keyring lookup in most cases, which opens up the future > possibilities of not even having the master key in memory following an > initial provisioning step (if the HMAC-SHA512 implementation is > hardware-backed), or of introducing an ioctl to provision a key to the > filesystem directly, avoiding keyrings and their visibility problems > entirely. Also, because key_hash is public information while raw_key is > secret information, it would have been very difficult to use raw_key as > a map key in a way that would prevent timing attacks while still being > scalable to a large number of entries. > > Signed-off-by: Eric Biggers Reviewed-by: Michael Halcrow > --- > fs/crypto/fscrypt_private.h | 11 > fs/crypto/keyinfo.c | 134 > +++- > fs/crypto/policy.c | 5 +- > fs/super.c | 4 ++ > include/linux/fs.h | 5 ++ > 5 files changed, 152 insertions(+), 7 deletions(-) > > diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h > index a7baeac92575..4b158717a8c3 100644 > --- a/fs/crypto/fscrypt_private.h > +++ b/fs/crypto/fscrypt_private.h > @@ -88,11 +88,22 @@ fscrypt_valid_context_format(const struct fscrypt_context > *ctx, int size) > > /* > * fscrypt_master_key - an in-use master key > + * > + * This is referenced from each in-core inode that has been "unlocked" using > a > + * particular master key. It's primarily used to cache the HMAC transform so > + * that the per-inode encryption keys can be derived efficiently with HKDF. > It > + * is securely erased once all inodes referencing it have been evicted. > + * > + * If the same master key is used on different filesystems (unusual, but > + * possible), we'll create one of these structs for each filesystem. > */ > struct fscrypt_master_key { > struct crypto_shash *mk_hmac; > unsigned intmk_size; > u8 mk_hash[FSCRYPT_KEY_HASH_SIZE]; > + refcount_t mk_refcount; > + struct rb_node mk_node; > + struct super_block *mk_sb; > }; > > /* > diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c > index 12a60eacf819..bf60e76f9599 100644 > --- a/fs/crypto/keyinfo.c > +++ b/fs/crypto/keyinfo.c > @@ -176,6 +176,14 @@ static void put_master_key(struct fscrypt_master_key *k) > if (!k) > return; > > + if (refcount_read(>mk_refcount) != 0) { /* in ->s_master_keys? */ > + if (!refcount_dec_and_lock(>mk_refcount, > +>mk_sb->s_master_keys_lock)) > + return; > + rb_erase(>mk_node, >mk_sb->s_master_keys); > + spin_unlock(>mk_sb->s_master_keys_lock); > + } > + > crypto_free_shash(k->mk_hmac); > kzfree(k); > } > @@ -231,6 +239,87 @@ alloc_master_key(const struct fscrypt_key *payload) > goto out; > } > > +/* > + * ->s_master_keys is a map of master keys currently in use by in-core > inodes on > + * a given filesystem, identified by key_hash which is a cryptographically > + * secure identifier for an actual key payload. > + * > + * Note that master_key_descriptor cannot be used to identify the keys > because > + * master_key_descriptor only identifies the "location" of a key in the > keyring, > +
[PATCH 5/6] fscrypt: cache the HMAC transform for each master key
From: Eric BiggersNow that we have a key_hash field which securely identifies a master key payload, introduce a cache of the HMAC transforms for the master keys currently in use for inodes using v2+ encryption policies. The entries in this cache are called 'struct fscrypt_master_key' and are identified by key_hash. The cache is per-superblock. (It could be global, but making it per-superblock should reduce the lock contention a bit, and we may need to keep track of keys on a per-superblock basis for other reasons later, e.g. to implement an ioctl for evicting keys.) This results in a large efficiency gain: we now only have to allocate and key an "hmac(sha512)" transformation, execute HKDF-Extract, and compute key_hash once per master key rather than once per inode. Note that this optimization can't easily be applied to the original AES-based KDF because that uses a different AES key for each KDF execution. In practice, this difference makes the HKDF per-inode encryption key derivation performance comparable to or even faster than the old KDF, which typically spends more time allocating an "ecb(aes)" transformation from the crypto API than doing actual crypto work. Note that it would have been possible to make the mapping be from raw_key => fscrypt_master_key (where raw_key denotes the actual bytes of the master key) rather than from key_hash => fscrypt_master_key. However, an advantage of doing lookups by key_hash is that it replaces the keyring lookup in most cases, which opens up the future possibilities of not even having the master key in memory following an initial provisioning step (if the HMAC-SHA512 implementation is hardware-backed), or of introducing an ioctl to provision a key to the filesystem directly, avoiding keyrings and their visibility problems entirely. Also, because key_hash is public information while raw_key is secret information, it would have been very difficult to use raw_key as a map key in a way that would prevent timing attacks while still being scalable to a large number of entries. Signed-off-by: Eric Biggers --- fs/crypto/fscrypt_private.h | 11 fs/crypto/keyinfo.c | 134 +++- fs/crypto/policy.c | 5 +- fs/super.c | 4 ++ include/linux/fs.h | 5 ++ 5 files changed, 152 insertions(+), 7 deletions(-) diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index a7baeac92575..4b158717a8c3 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -88,11 +88,22 @@ fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size) /* * fscrypt_master_key - an in-use master key + * + * This is referenced from each in-core inode that has been "unlocked" using a + * particular master key. It's primarily used to cache the HMAC transform so + * that the per-inode encryption keys can be derived efficiently with HKDF. It + * is securely erased once all inodes referencing it have been evicted. + * + * If the same master key is used on different filesystems (unusual, but + * possible), we'll create one of these structs for each filesystem. */ struct fscrypt_master_key { struct crypto_shash *mk_hmac; unsigned intmk_size; u8 mk_hash[FSCRYPT_KEY_HASH_SIZE]; + refcount_t mk_refcount; + struct rb_node mk_node; + struct super_block *mk_sb; }; /* diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c index 12a60eacf819..bf60e76f9599 100644 --- a/fs/crypto/keyinfo.c +++ b/fs/crypto/keyinfo.c @@ -176,6 +176,14 @@ static void put_master_key(struct fscrypt_master_key *k) if (!k) return; + if (refcount_read(>mk_refcount) != 0) { /* in ->s_master_keys? */ + if (!refcount_dec_and_lock(>mk_refcount, + >mk_sb->s_master_keys_lock)) + return; + rb_erase(>mk_node, >mk_sb->s_master_keys); + spin_unlock(>mk_sb->s_master_keys_lock); + } + crypto_free_shash(k->mk_hmac); kzfree(k); } @@ -231,6 +239,87 @@ alloc_master_key(const struct fscrypt_key *payload) goto out; } +/* + * ->s_master_keys is a map of master keys currently in use by in-core inodes on + * a given filesystem, identified by key_hash which is a cryptographically + * secure identifier for an actual key payload. + * + * Note that master_key_descriptor cannot be used to identify the keys because + * master_key_descriptor only identifies the "location" of a key in the keyring, + * not the actual key payload --- i.e., buggy or malicious userspace may provide + * different keys with the same master_key_descriptor. + */ + +/* + * Search ->s_master_keys for the fscrypt_master_key having the specified hash. + * If found return it with a reference taken, otherwise return NULL. +