2016-11-27 04:42:12

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 1/4] fscrypt: rename get_crypt_info() to fscrypt_get_crypt_info()

To avoid namespace collisions, rename get_crypt_info() to
fscrypt_get_crypt_info(). The function is only used inside the
fs/crypto directory, so declare it in the new header file,
fscrypt_private.h.

Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/crypto/fname.c | 4 ++--
fs/crypto/fscrypt_private.h | 19 +++++++++++++++++++
fs/crypto/keyinfo.c | 6 +++---
include/linux/fscrypto.h | 1 -
4 files changed, 24 insertions(+), 6 deletions(-)
create mode 100644 fs/crypto/fscrypt_private.h

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 6b45d9caeeb0..56ad9d195f18 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -12,7 +12,7 @@

#include <linux/scatterlist.h>
#include <linux/ratelimit.h>
-#include <linux/fscrypto.h>
+#include "fscrypt_private.h"

/**
* fname_crypt_complete() - completion callback for filename crypto
@@ -350,7 +350,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
fname->disk_name.len = iname->len;
return 0;
}
- ret = get_crypt_info(dir);
+ ret = fscrypt_get_crypt_info(dir);
if (ret && ret != -EOPNOTSUPP)
return ret;

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
new file mode 100644
index 000000000000..7c31108728e4
--- /dev/null
+++ b/fs/crypto/fscrypt_private.h
@@ -0,0 +1,19 @@
+/*
+ * fscrypt_private.h
+ *
+ * Copyright (C) 2015, Google, Inc.
+ *
+ * This contains encryption key functions.
+ *
+ * Written by Michael Halcrow, Ildar Muslukhov, and Uday Savagaonkar, 2015.
+ */
+
+#ifndef _FSCRYPT_PRIVATE_H
+#define _FSCRYPT_PRIVATE_H
+
+#include <linux/fscrypto.h>
+
+/* keyinfo.c */
+extern int fscrypt_get_crypt_info(struct inode *);
+
+#endif /* _FSCRYPT_PRIVATE_H */
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 67fb6d8876d0..35d3317a27b3 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -10,7 +10,7 @@

#include <keys/user-type.h>
#include <linux/scatterlist.h>
-#include <linux/fscrypto.h>
+#include "fscrypt_private.h"

static void derive_crypt_complete(struct crypto_async_request *req, int rc)
{
@@ -178,7 +178,7 @@ static void put_crypt_info(struct fscrypt_info *ci)
kmem_cache_free(fscrypt_info_cachep, ci);
}

-int get_crypt_info(struct inode *inode)
+int fscrypt_get_crypt_info(struct inode *inode)
{
struct fscrypt_info *crypt_info;
struct fscrypt_context ctx;
@@ -327,7 +327,7 @@ int fscrypt_get_encryption_info(struct inode *inode)
(ci->ci_keyring_key->flags & ((1 << KEY_FLAG_INVALIDATED) |
(1 << KEY_FLAG_REVOKED) |
(1 << KEY_FLAG_DEAD)))))
- return get_crypt_info(inode);
+ return fscrypt_get_crypt_info(inode);
return 0;
}
EXPORT_SYMBOL(fscrypt_get_encryption_info);
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index be94684dc05f..2f8894f0696c 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -265,7 +265,6 @@ extern int fscrypt_has_permitted_context(struct inode *, struct inode *);
extern int fscrypt_inherit_context(struct inode *, struct inode *,
void *, bool);
/* keyinfo.c */
-extern int get_crypt_info(struct inode *);
extern int fscrypt_get_encryption_info(struct inode *);
extern void fscrypt_put_encryption_info(struct inode *, struct fscrypt_info *);

--
2.11.0.rc0.7.gbe5a750



2016-11-27 04:42:12

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 2/4] fscrypt: unexport fscrypt_initialize()

The fscrypt_initalize() function isn't used outside fs/crypto, so
there's no point making it be an exported symbol.

Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/crypto/crypto.c | 1 -
fs/crypto/fscrypt_private.h | 3 +++
include/linux/fscrypto.h | 1 -
3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index b6029785714c..56f98f45cece 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -540,7 +540,6 @@ int fscrypt_initialize(void)
mutex_unlock(&fscrypt_init_mutex);
return res;
}
-EXPORT_SYMBOL(fscrypt_initialize);

/**
* fscrypt_init() - Set up for fs encryption.
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 7c31108728e4..bb92f0c0961b 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -13,6 +13,9 @@

#include <linux/fscrypto.h>

+/* crypto.c */
+int fscrypt_initialize(void);
+
/* keyinfo.c */
extern int fscrypt_get_crypt_info(struct inode *);

diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index 2f8894f0696c..ce2ebdee6a89 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -244,7 +244,6 @@ static inline void fscrypt_set_d_op(struct dentry *dentry)
#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
/* crypto.c */
extern struct kmem_cache *fscrypt_info_cachep;
-int fscrypt_initialize(void);

extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);
extern void fscrypt_release_ctx(struct fscrypt_ctx *);
--
2.11.0.rc0.7.gbe5a750


2016-11-27 04:42:12

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 4/4] fscrypt: move the policy flags and encryption mode definitions to uapi header

These constants are part of the UAPI, so they belong in
include/uapi/linux/fs.h instead of include/linux/fscrypto.h

Signed-off-by: Theodore Ts'o <[email protected]>
---
include/linux/fscrypto.h | 14 --------------
include/uapi/linux/fs.h | 14 ++++++++++++++
2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index 71e8a20711ec..42ef82d60790 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -18,20 +18,6 @@
#include <crypto/skcipher.h>
#include <uapi/linux/fs.h>

-#define FS_POLICY_FLAGS_PAD_4 0x00
-#define FS_POLICY_FLAGS_PAD_8 0x01
-#define FS_POLICY_FLAGS_PAD_16 0x02
-#define FS_POLICY_FLAGS_PAD_32 0x03
-#define FS_POLICY_FLAGS_PAD_MASK 0x03
-#define FS_POLICY_FLAGS_VALID 0x03
-
-/* Encryption algorithms */
-#define FS_ENCRYPTION_MODE_INVALID 0
-#define FS_ENCRYPTION_MODE_AES_256_XTS 1
-#define FS_ENCRYPTION_MODE_AES_256_GCM 2
-#define FS_ENCRYPTION_MODE_AES_256_CBC 3
-#define FS_ENCRYPTION_MODE_AES_256_CTS 4
-
#define FS_CRYPTO_BLOCK_SIZE 16

struct fscrypt_info;
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index acb2b6152ba0..0496d37abe28 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -254,6 +254,20 @@ struct fsxattr {
/* Policy provided via an ioctl on the topmost directory */
#define FS_KEY_DESCRIPTOR_SIZE 8

+#define FS_POLICY_FLAGS_PAD_4 0x00
+#define FS_POLICY_FLAGS_PAD_8 0x01
+#define FS_POLICY_FLAGS_PAD_16 0x02
+#define FS_POLICY_FLAGS_PAD_32 0x03
+#define FS_POLICY_FLAGS_PAD_MASK 0x03
+#define FS_POLICY_FLAGS_VALID 0x03
+
+/* Encryption algorithms */
+#define FS_ENCRYPTION_MODE_INVALID 0
+#define FS_ENCRYPTION_MODE_AES_256_XTS 1
+#define FS_ENCRYPTION_MODE_AES_256_GCM 2
+#define FS_ENCRYPTION_MODE_AES_256_CBC 3
+#define FS_ENCRYPTION_MODE_AES_256_CTS 4
+
struct fscrypt_policy {
__u8 version;
__u8 contents_encryption_mode;
--
2.11.0.rc0.7.gbe5a750


2016-11-27 04:42:12

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH 3/4] fscrypt: move non-public structures and constants to fscrypt_private.h

Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/crypto/crypto.c | 2 +-
fs/crypto/fscrypt_private.h | 71 +++++++++++++++++++++++++++++++++++++++++++++
fs/crypto/policy.c | 2 +-
include/linux/fscrypto.h | 68 ++-----------------------------------------
4 files changed, 76 insertions(+), 67 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 56f98f45cece..4d9d221b1d60 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -27,7 +27,7 @@
#include <linux/bio.h>
#include <linux/dcache.h>
#include <linux/namei.h>
-#include <linux/fscrypto.h>
+#include "fscrypt_private.h"

static unsigned int num_prealloc_crypto_pages = 32;
static unsigned int num_prealloc_crypto_ctxs = 128;
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index bb92f0c0961b..c98b2a7fb6d3 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -13,6 +13,77 @@

#include <linux/fscrypto.h>

+#define FS_FNAME_CRYPTO_DIGEST_SIZE 32
+
+/* Encryption parameters */
+#define FS_XTS_TWEAK_SIZE 16
+#define FS_AES_128_ECB_KEY_SIZE 16
+#define FS_AES_256_GCM_KEY_SIZE 32
+#define FS_AES_256_CBC_KEY_SIZE 32
+#define FS_AES_256_CTS_KEY_SIZE 32
+#define FS_AES_256_XTS_KEY_SIZE 64
+#define FS_MAX_KEY_SIZE 64
+
+#define FS_KEY_DESC_PREFIX "fscrypt:"
+#define FS_KEY_DESC_PREFIX_SIZE 8
+
+#define FS_KEY_DERIVATION_NONCE_SIZE 16
+
+/**
+ * Encryption context for inode
+ *
+ * Protector format:
+ * 1 byte: Protector format (1 = this version)
+ * 1 byte: File contents encryption mode
+ * 1 byte: File names encryption mode
+ * 1 byte: Flags
+ * 8 bytes: Master Key descriptor
+ * 16 bytes: Encryption Key derivation nonce
+ */
+struct fscrypt_context {
+ u8 format;
+ u8 contents_encryption_mode;
+ u8 filenames_encryption_mode;
+ u8 flags;
+ u8 master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
+ u8 nonce[FS_KEY_DERIVATION_NONCE_SIZE];
+} __packed;
+
+#define FS_ENCRYPTION_CONTEXT_FORMAT_V1 1
+
+/* This is passed in from userspace into the kernel keyring */
+struct fscrypt_key {
+ u32 mode;
+ u8 raw[FS_MAX_KEY_SIZE];
+ u32 size;
+} __packed;
+
+/*
+ * A pointer to this structure is stored in the file system's in-core
+ * representation of an inode.
+ */
+struct fscrypt_info {
+ u8 ci_data_mode;
+ u8 ci_filename_mode;
+ u8 ci_flags;
+ struct crypto_skcipher *ci_ctfm;
+ struct key *ci_keyring_key;
+ u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
+};
+
+#define FS_CTX_REQUIRES_FREE_ENCRYPT_FL 0x00000001
+#define FS_WRITE_PATH_FL 0x00000002
+
+struct fscrypt_completion_result {
+ struct completion completion;
+ int res;
+};
+
+#define DECLARE_FS_COMPLETION_RESULT(ecr) \
+ struct fscrypt_completion_result ecr = { \
+ COMPLETION_INITIALIZER((ecr).completion), 0 }
+
+
/* crypto.c */
int fscrypt_initialize(void);

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index b96a10e3cf78..6ed7c2eebeec 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -10,8 +10,8 @@

#include <linux/random.h>
#include <linux/string.h>
-#include <linux/fscrypto.h>
#include <linux/mount.h>
+#include "fscrypt_private.h"

static int inode_has_encryption_context(struct inode *inode)
{
diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h
index ce2ebdee6a89..71e8a20711ec 100644
--- a/include/linux/fscrypto.h
+++ b/include/linux/fscrypto.h
@@ -18,9 +18,6 @@
#include <crypto/skcipher.h>
#include <uapi/linux/fs.h>

-#define FS_KEY_DERIVATION_NONCE_SIZE 16
-#define FS_ENCRYPTION_CONTEXT_FORMAT_V1 1
-
#define FS_POLICY_FLAGS_PAD_4 0x00
#define FS_POLICY_FLAGS_PAD_8 0x01
#define FS_POLICY_FLAGS_PAD_16 0x02
@@ -35,56 +32,10 @@
#define FS_ENCRYPTION_MODE_AES_256_CBC 3
#define FS_ENCRYPTION_MODE_AES_256_CTS 4

-/**
- * Encryption context for inode
- *
- * Protector format:
- * 1 byte: Protector format (1 = this version)
- * 1 byte: File contents encryption mode
- * 1 byte: File names encryption mode
- * 1 byte: Flags
- * 8 bytes: Master Key descriptor
- * 16 bytes: Encryption Key derivation nonce
- */
-struct fscrypt_context {
- u8 format;
- u8 contents_encryption_mode;
- u8 filenames_encryption_mode;
- u8 flags;
- u8 master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
- u8 nonce[FS_KEY_DERIVATION_NONCE_SIZE];
-} __packed;
-
-/* Encryption parameters */
-#define FS_XTS_TWEAK_SIZE 16
-#define FS_AES_128_ECB_KEY_SIZE 16
-#define FS_AES_256_GCM_KEY_SIZE 32
-#define FS_AES_256_CBC_KEY_SIZE 32
-#define FS_AES_256_CTS_KEY_SIZE 32
-#define FS_AES_256_XTS_KEY_SIZE 64
-#define FS_MAX_KEY_SIZE 64
-
-#define FS_KEY_DESC_PREFIX "fscrypt:"
-#define FS_KEY_DESC_PREFIX_SIZE 8
-
-/* This is passed in from userspace into the kernel keyring */
-struct fscrypt_key {
- u32 mode;
- u8 raw[FS_MAX_KEY_SIZE];
- u32 size;
-} __packed;
-
-struct fscrypt_info {
- u8 ci_data_mode;
- u8 ci_filename_mode;
- u8 ci_flags;
- struct crypto_skcipher *ci_ctfm;
- struct key *ci_keyring_key;
- u8 ci_master_key[FS_KEY_DESCRIPTOR_SIZE];
-};
+#define FS_CRYPTO_BLOCK_SIZE 16

-#define FS_CTX_REQUIRES_FREE_ENCRYPT_FL 0x00000001
-#define FS_WRITE_PATH_FL 0x00000002
+struct fscrypt_info;
+struct fscrypt_ctx;

struct fscrypt_ctx {
union {
@@ -102,19 +53,6 @@ struct fscrypt_ctx {
u8 mode; /* Encryption mode for tfm */
};

-struct fscrypt_completion_result {
- struct completion completion;
- int res;
-};
-
-#define DECLARE_FS_COMPLETION_RESULT(ecr) \
- struct fscrypt_completion_result ecr = { \
- COMPLETION_INITIALIZER((ecr).completion), 0 }
-
-#define FS_FNAME_NUM_SCATTER_ENTRIES 4
-#define FS_CRYPTO_BLOCK_SIZE 16
-#define FS_FNAME_CRYPTO_DIGEST_SIZE 32

2016-11-29 21:00:37

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 1/4] fscrypt: rename get_crypt_info() to fscrypt_get_crypt_info()

On Sat, Nov 26, 2016 at 11:41:52PM -0500, Theodore Ts'o wrote:
> To avoid namespace collisions, rename get_crypt_info() to
> fscrypt_get_crypt_info(). The function is only used inside the
> fs/crypto directory, so declare it in the new header file,
> fscrypt_private.h.
>
> Signed-off-by: Theodore Ts'o <[email protected]>

Reviewed-by: Eric Biggers <[email protected]>

2016-11-29 21:01:45

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 2/4] fscrypt: unexport fscrypt_initialize()

On Sat, Nov 26, 2016 at 11:41:53PM -0500, Theodore Ts'o wrote:
> The fscrypt_initalize() function isn't used outside fs/crypto, so
> there's no point making it be an exported symbol.
>
> Signed-off-by: Theodore Ts'o <[email protected]>

Reviewed-by: Eric Biggers <[email protected]>

2016-11-29 21:07:02

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 3/4] fscrypt: move non-public structures and constants to fscrypt_private.h

On Sat, Nov 26, 2016 at 11:41:54PM -0500, Theodore Ts'o wrote:
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> fs/crypto/crypto.c | 2 +-
> fs/crypto/fscrypt_private.h | 71 +++++++++++++++++++++++++++++++++++++++++++++
> fs/crypto/policy.c | 2 +-
> include/linux/fscrypto.h | 68 ++-----------------------------------------
> 4 files changed, 76 insertions(+), 67 deletions(-)
>

This looks good to me. There are however a few other things I think would
belong in the private header too, like the #includes of <linux/key.h> and
<crypto/skcipher.h>, the declaration of 'fscrypt_info_cachep', and some of the
inline functions.

Reviewed-by: Eric Biggers <[email protected]>

2016-11-29 21:30:37

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 4/4] fscrypt: move the policy flags and encryption mode definitions to uapi header

On Sat, Nov 26, 2016 at 11:41:55PM -0500, Theodore Ts'o wrote:
> These constants are part of the UAPI, so they belong in
> include/uapi/linux/fs.h instead of include/linux/fscrypto.h
>
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> include/linux/fscrypto.h | 14 --------------
> include/uapi/linux/fs.h | 14 ++++++++++++++
> 2 files changed, 14 insertions(+), 14 deletions(-)
>

This looks fine to me. I had wanted to do this a while back, but there was some
talk about exposing the modes under names that hide the specific algorithms,
like "CRYPT_SW_V1", with the assumption being that users shouldn't have to know
about any specific modes but rather simply choose the modes with the highest
version numbers, which would also be the most secure. But I wasn't really
convinced by that argument because people might add or use specific modes for
reasons other than security, such as hardware support on a certain platform.
And I think a better solution to the "use the most secure mode the kernel knows
about" problem would be to implement a special mode like
FS_ENCRYPTION_MODE_MOST_SECURE which the kernel would translate into its
preferred mode when setting an encryption policy.

Mike, do you have a different opinion?

I should also mention that the UAPI header is also missing struct fscrypt_key
and its the definitions FS_MAX_KEY_SIZE, FS_KEY_DESC_PREFIX, and
FS_KEY_DESC_PREFIX_SIZE. I think those should be moved to the UAPI header too,
though that can be a separate patch.

Reviewed-by: Eric Biggers <[email protected]>

Eric