[PATCH v2 00/11] fscrypt: symlink helpers and fscrypt.h cleanup

[PATCH v2 00/11] fscrypt: symlink helpers and fscrypt.h cleanup

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

This series introduces helper functions in fscrypt that filesystems can
call to simplify handling of encrypted symlinks.  The helpers also fix a
couple subtle bugs, such as incorrectly rejecting symlinks that are very
close to the maximum length, and accidentally leaving the "." and ".."
symlink targets unencrypted (they need to be special in directory
entries, but not in symlink targets).

Patches 1-9 are mostly cleanup to trim down fscrypt.h, moving stuff into
more appropriate places depending on whether it is needed only
internally by fscrypt, or only by filesystems supporting encryption,
etc.  I was going to send these out as a separate series, but there is a
dependency because the symlink helpers depend on
fscrypt_dummy_context_enabled() having been fixed to work correctly in
the "notsupp" case.

Patch 10 introduces the helpers for ->symlink() and patch 11 introduces
a helper for ->get_link().

The patches to switch ext4, f2fs, and ubifs over to use the helper
functions were included in v1 of this patchset
(https://marc.info/?l=linux-ext4&m=151336000031869).  They are not
included now since I'm planning to get them picked up by the individual
filesystem maintainers after this goes in.

Changed since v1:

    - Fixed __fscrypt_prepare_symlink() to work correctly with
      test_dummy_encryption mode in the case where the directory is
      unencrypted and has not yet had its ->i_crypt_info set up.

Eric Biggers (11):
  fscrypt: move fscrypt_has_encryption_key() to supp/notsupp headers
  fscrypt: move fscrypt_control_page() to supp/notsupp headers
  fscrypt: move fscrypt_info_cachep declaration to fscrypt_private.h
  fscrypt: move fscrypt_ctx declaration to fscrypt_supp.h
  fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp
    versions
  fscrypt: move fscrypt_operations declaration to fscrypt_supp.h
  fscrypt: move fscrypt_valid_enc_modes() to fscrypt_private.h
  fscrypt: move fscrypt_is_dot_dotdot() to fs/crypto/fname.c
  fscrypt: trim down fscrypt.h includes
  fscrypt: new helper functions for ->symlink()
  fscrypt: new helper function - fscrypt_get_symlink()

 fs/crypto/crypto.c              |   1 +
 fs/crypto/fname.c               |  20 ++++-
 fs/crypto/fscrypt_private.h     |  19 +++++
 fs/crypto/hooks.c               | 163 +++++++++++++++++++++++++++++++++++++++
 fs/crypto/keyinfo.c             |   1 +
 include/linux/fscrypt.h         | 165 ++++++++++++++++------------------------
 include/linux/fscrypt_notsupp.h |  39 ++++++++++
 include/linux/fscrypt_supp.h    |  63 ++++++++++++++-
 8 files changed, 369 insertions(+), 102 deletions(-)

-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 01/11] fscrypt: move fscrypt_has_encryption_key() to supp/notsupp headers

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

fscrypt_has_encryption_key() is already split into two versions
depending on whether the filesystem is being built with encryption
support or not.  Move them into the appropriate headers.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/fscrypt.h         | 10 ----------
 include/linux/fscrypt_notsupp.h |  5 +++++
 include/linux/fscrypt_supp.h    |  5 +++++
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 08b4b40c5aa8..d1c891b5bd9c 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -129,11 +129,6 @@ static inline struct page *fscrypt_control_page(struct page *page)
 	return ((struct fscrypt_ctx *)page_private(page))->w.control_page;
 }
 
-static inline bool fscrypt_has_encryption_key(const struct inode *inode)
-{
-	return (inode->i_crypt_info != NULL);
-}
-
 #include <linux/fscrypt_supp.h>
 
 #else /* !__FS_HAS_ENCRYPTION */
@@ -144,11 +139,6 @@ static inline struct page *fscrypt_control_page(struct page *page)
 	return ERR_PTR(-EINVAL);
 }
 
-static inline bool fscrypt_has_encryption_key(const struct inode *inode)
-{
-	return 0;
-}
-
 #include <linux/fscrypt_notsupp.h>
 #endif /* __FS_HAS_ENCRYPTION */
 
diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h
index 63e58808519a..52e330285457 100644
--- a/include/linux/fscrypt_notsupp.h
+++ b/include/linux/fscrypt_notsupp.h
@@ -14,6 +14,11 @@
 #ifndef _LINUX_FSCRYPT_NOTSUPP_H
 #define _LINUX_FSCRYPT_NOTSUPP_H
 
+static inline bool fscrypt_has_encryption_key(const struct inode *inode)
+{
+	return false;
+}
+
 /* crypto.c */
 static inline struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *inode,
 						  gfp_t gfp_flags)
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index cf9e9fc02f0a..79bb8beae018 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -11,6 +11,11 @@
 #ifndef _LINUX_FSCRYPT_SUPP_H
 #define _LINUX_FSCRYPT_SUPP_H
 
+static inline bool fscrypt_has_encryption_key(const struct inode *inode)
+{
+	return (inode->i_crypt_info != NULL);
+}
+
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
 extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 02/11] fscrypt: move fscrypt_control_page() to supp/notsupp headers

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

fscrypt_control_page() is already split into two versions depending on
whether the filesystem is being built with encryption support or not.
Move them into the appropriate headers.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/fscrypt.h         | 18 ++----------------
 include/linux/fscrypt_notsupp.h |  5 +++++
 include/linux/fscrypt_supp.h    |  6 ++++++
 3 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index d1c891b5bd9c..c23b2f16129a 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -123,24 +123,10 @@ static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
 }
 
 #if __FS_HAS_ENCRYPTION
-
-static inline struct page *fscrypt_control_page(struct page *page)
-{
-	return ((struct fscrypt_ctx *)page_private(page))->w.control_page;
-}
-
 #include <linux/fscrypt_supp.h>
-
-#else /* !__FS_HAS_ENCRYPTION */
-
-static inline struct page *fscrypt_control_page(struct page *page)
-{
-	WARN_ON_ONCE(1);
-	return ERR_PTR(-EINVAL);
-}
-
+#else
 #include <linux/fscrypt_notsupp.h>
-#endif /* __FS_HAS_ENCRYPTION */
+#endif
 
 /**
  * fscrypt_require_key - require an inode's encryption key
diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h
index 52e330285457..812dc701a5b3 100644
--- a/include/linux/fscrypt_notsupp.h
+++ b/include/linux/fscrypt_notsupp.h
@@ -48,6 +48,11 @@ static inline int fscrypt_decrypt_page(const struct inode *inode,
 	return -EOPNOTSUPP;
 }
 
+static inline struct page *fscrypt_control_page(struct page *page)
+{
+	WARN_ON_ONCE(1);
+	return ERR_PTR(-EINVAL);
+}
 
 static inline void fscrypt_restore_control_page(struct page *page)
 {
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index 79bb8beae018..ad40780d4653 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -25,6 +25,12 @@ extern struct page *fscrypt_encrypt_page(const struct inode *, struct page *,
 						u64, gfp_t);
 extern int fscrypt_decrypt_page(const struct inode *, struct page *, unsigned int,
 				unsigned int, u64);
+
+static inline struct page *fscrypt_control_page(struct page *page)
+{
+	return ((struct fscrypt_ctx *)page_private(page))->w.control_page;
+}
+
 extern void fscrypt_restore_control_page(struct page *);
 
 extern const struct dentry_operations fscrypt_d_ops;
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 03/11] fscrypt: move fscrypt_info_cachep declaration to fscrypt_private.h

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The fscrypt_info kmem_cache is internal to fscrypt; filesystems don't
need to access it.  So move its declaration into fscrypt_private.h.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h  | 1 +
 include/linux/fscrypt_supp.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index c0b4f5597e1a..823c43a00bf7 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -72,6 +72,7 @@ typedef enum {
 #define FS_CTX_HAS_BOUNCE_BUFFER_FL		0x00000002
 
 /* crypto.c */
+extern struct kmem_cache *fscrypt_info_cachep;
 extern int fscrypt_initialize(unsigned int cop_flags);
 extern struct workqueue_struct *fscrypt_read_workqueue;
 extern int fscrypt_do_page_crypto(const struct inode *inode,
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index ad40780d4653..33d641e27c18 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -17,7 +17,6 @@ static inline bool fscrypt_has_encryption_key(const struct inode *inode)
 }
 
 /* crypto.c */
-extern struct kmem_cache *fscrypt_info_cachep;
 extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);
 extern void fscrypt_release_ctx(struct fscrypt_ctx *);
 extern struct page *fscrypt_encrypt_page(const struct inode *, struct page *,
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 04/11] fscrypt: move fscrypt_ctx declaration to fscrypt_supp.h

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Filesystems only ever access 'struct fscrypt_ctx' through fscrypt
functions.  But when a filesystem is built without encryption support,
these functions are all stubbed out, so the declaration of fscrypt_ctx
is unneeded.  Therefore, move it from fscrypt.h to fscrypt_supp.h.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/fscrypt.h      | 16 +---------------
 include/linux/fscrypt_supp.h | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index c23b2f16129a..0f94d087a6d1 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -24,23 +24,9 @@
 
 #define FS_CRYPTO_BLOCK_SIZE		16
 
+struct fscrypt_ctx;
 struct fscrypt_info;
 
-struct fscrypt_ctx {
-	union {
-		struct {
-			struct page *bounce_page;	/* Ciphertext page */
-			struct page *control_page;	/* Original page  */
-		} w;
-		struct {
-			struct bio *bio;
-			struct work_struct work;
-		} r;
-		struct list_head free_list;	/* Free list */
-	};
-	u8 flags;				/* Flags */
-};
-
 /**
  * For encrypted symlinks, the ciphertext length is stored at the beginning
  * of the string in little-endian format.
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index 33d641e27c18..fd6ee089ced0 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -11,6 +11,21 @@
 #ifndef _LINUX_FSCRYPT_SUPP_H
 #define _LINUX_FSCRYPT_SUPP_H
 
+struct fscrypt_ctx {
+	union {
+		struct {
+			struct page *bounce_page;	/* Ciphertext page */
+			struct page *control_page;	/* Original page  */
+		} w;
+		struct {
+			struct bio *bio;
+			struct work_struct work;
+		} r;
+		struct list_head free_list;	/* Free list */
+	};
+	u8 flags;				/* Flags */
+};
+
 static inline bool fscrypt_has_encryption_key(const struct inode *inode)
 {
 	return (inode->i_crypt_info != NULL);
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 05/11] fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp versions

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

fscrypt_dummy_context_enabled() accesses ->s_cop, which now is only set
when the filesystem is built with encryption support.  This didn't
actually matter because no filesystems called it.  However, it will
start being used soon, so fix it by moving it from fscrypt.h to
fscrypt_supp.h and stubbing it out in fscrypt_notsupp.h.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/fscrypt.h         | 8 --------
 include/linux/fscrypt_notsupp.h | 5 +++++
 include/linux/fscrypt_supp.h    | 6 ++++++
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 0f94d087a6d1..b671a4eef47f 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -75,14 +75,6 @@ struct fscrypt_operations {
 /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
 #define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
 
-static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
-{
-	if (inode->i_sb->s_cop->dummy_context &&
-				inode->i_sb->s_cop->dummy_context(inode))
-		return true;
-	return false;
-}
-
 static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
 					u32 filenames_mode)
 {
diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h
index 812dc701a5b3..81e02201b215 100644
--- a/include/linux/fscrypt_notsupp.h
+++ b/include/linux/fscrypt_notsupp.h
@@ -19,6 +19,11 @@ static inline bool fscrypt_has_encryption_key(const struct inode *inode)
 	return false;
 }
 
+static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
+{
+	return false;
+}
+
 /* crypto.c */
 static inline struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *inode,
 						  gfp_t gfp_flags)
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index fd6ee089ced0..e7dfa2974906 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -31,6 +31,12 @@ static inline bool fscrypt_has_encryption_key(const struct inode *inode)
 	return (inode->i_crypt_info != NULL);
 }
 
+static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
+{
+	return inode->i_sb->s_cop->dummy_context &&
+		inode->i_sb->s_cop->dummy_context(inode);
+}
+
 /* crypto.c */
 extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);
 extern void fscrypt_release_ctx(struct fscrypt_ctx *);
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCH v2 05/11] fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp versions

[Jaegeuk Kim]

On 01/05, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> fscrypt_dummy_context_enabled() accesses ->s_cop, which now is only set
> when the filesystem is built with encryption support.  This didn't
> actually matter because no filesystems called it.  However, it will
> start being used soon, so fix it by moving it from fscrypt.h to
> fscrypt_supp.h and stubbing it out in fscrypt_notsupp.h.

Ted, do we have a chance to get rid of this dummy_context? If there exists
backward compatibility issue, please never mind tho.

Thanks,

> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  include/linux/fscrypt.h         | 8 --------
>  include/linux/fscrypt_notsupp.h | 5 +++++
>  include/linux/fscrypt_supp.h    | 6 ++++++
>  3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index 0f94d087a6d1..b671a4eef47f 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -75,14 +75,6 @@ struct fscrypt_operations {
>  /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
>  #define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
>  
> -static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
> -{
> -	if (inode->i_sb->s_cop->dummy_context &&
> -				inode->i_sb->s_cop->dummy_context(inode))
> -		return true;
> -	return false;
> -}
> -
>  static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
>  					u32 filenames_mode)
>  {
> diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h
> index 812dc701a5b3..81e02201b215 100644
> --- a/include/linux/fscrypt_notsupp.h
> +++ b/include/linux/fscrypt_notsupp.h
> @@ -19,6 +19,11 @@ static inline bool fscrypt_has_encryption_key(const struct inode *inode)
>  	return false;
>  }
>  
> +static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
> +{
> +	return false;
> +}
> +
>  /* crypto.c */
>  static inline struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *inode,
>  						  gfp_t gfp_flags)
> diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
> index fd6ee089ced0..e7dfa2974906 100644
> --- a/include/linux/fscrypt_supp.h
> +++ b/include/linux/fscrypt_supp.h
> @@ -31,6 +31,12 @@ static inline bool fscrypt_has_encryption_key(const struct inode *inode)
>  	return (inode->i_crypt_info != NULL);
>  }
>  
> +static inline bool fscrypt_dummy_context_enabled(struct inode *inode)
> +{
> +	return inode->i_sb->s_cop->dummy_context &&
> +		inode->i_sb->s_cop->dummy_context(inode);
> +}
> +
>  /* crypto.c */
>  extern struct fscrypt_ctx *fscrypt_get_ctx(const struct inode *, gfp_t);
>  extern void fscrypt_release_ctx(struct fscrypt_ctx *);
> -- 
> 2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCH v2 05/11] fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp versions

[Eric Biggers]

On Fri, Jan 05, 2018 at 12:40:09PM -0800, Jaegeuk Kim wrote:
> On 01/05, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > fscrypt_dummy_context_enabled() accesses ->s_cop, which now is only set
> > when the filesystem is built with encryption support.  This didn't
> > actually matter because no filesystems called it.  However, it will
> > start being used soon, so fix it by moving it from fscrypt.h to
> > fscrypt_supp.h and stubbing it out in fscrypt_notsupp.h.
> 
> Ted, do we have a chance to get rid of this dummy_context? If there exists
> backward compatibility issue, please never mind tho.
> 

It's used to implement the test_dummy_encryption mount option for ext4, which is
used by the 'ext4/encrypt' config for gce-xfstests.  Its purpose is to cause all
new files (directories, regular files, and symlinks) to be automatically
encrypted with a default key, so that the encrypted I/O paths are tested more
thoroughly than by just running the 'encrypt' group tests.

There are no backward compatibility concerns with changing or removing the
test_dummy_encryption mount option; we just don't have a better solution yet.

Ideally, instead of using test_dummy_encryption we would encrypt the root
directory of the filesystem immediately after it is formatted.  However, that
doesn't work for ext4 because the lost+found directory has to be located in the
root directory, and must be unencrypted, and the lost+found directory entry must
be unencrypted.

So I think getting rid of test_dummy_encryption depends on a solution to the
lost+found problem.

An alternative would be to add support for "inherit-only" encryption policies,
then set such a policy on the root directory of the filesystem.  But, there
haven't been any requests for such a feature yet outside of this specific
testing-only use case, so I've been hesitant to add it.

Eric

Re: [PATCH v2 05/11] fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp versions

[Jaegeuk Kim]

On 01/05, Eric Biggers wrote:
> On Fri, Jan 05, 2018 at 12:40:09PM -0800, Jaegeuk Kim wrote:
> > On 01/05, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > fscrypt_dummy_context_enabled() accesses ->s_cop, which now is only set
> > > when the filesystem is built with encryption support.  This didn't
> > > actually matter because no filesystems called it.  However, it will
> > > start being used soon, so fix it by moving it from fscrypt.h to
> > > fscrypt_supp.h and stubbing it out in fscrypt_notsupp.h.
> > 
> > Ted, do we have a chance to get rid of this dummy_context? If there exists
> > backward compatibility issue, please never mind tho.
> > 
> 
> It's used to implement the test_dummy_encryption mount option for ext4, which is
> used by the 'ext4/encrypt' config for gce-xfstests.  Its purpose is to cause all
> new files (directories, regular files, and symlinks) to be automatically
> encrypted with a default key, so that the encrypted I/O paths are tested more
> thoroughly than by just running the 'encrypt' group tests.
> 
> There are no backward compatibility concerns with changing or removing the
> test_dummy_encryption mount option; we just don't have a better solution yet.
> 
> Ideally, instead of using test_dummy_encryption we would encrypt the root
> directory of the filesystem immediately after it is formatted.  However, that
> doesn't work for ext4 because the lost+found directory has to be located in the
> root directory, and must be unencrypted, and the lost+found directory entry must
> be unencrypted.
> 
> So I think getting rid of test_dummy_encryption depends on a solution to the
> lost+found problem.

Thank you for the explanation. Actually, I've got used to encrypt the root dir
in f2fs for such the testing purpose, since it's indeed empty. If it's the only
matter of lost_found in ext4, how about setting the encryption bit and dummy
context in root inode by mke2fs or tune2fs?

Thanks,

> 
> An alternative would be to add support for "inherit-only" encryption policies,
> then set such a policy on the root directory of the filesystem.  But, there
> haven't been any requests for such a feature yet outside of this specific
> testing-only use case, so I've been hesitant to add it.
> 
> Eric

Re: [PATCH v2 05/11] fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp versions

[Eric Biggers]

On Fri, Jan 05, 2018 at 01:36:08PM -0800, Jaegeuk Kim wrote:
> On 01/05, Eric Biggers wrote:
> > On Fri, Jan 05, 2018 at 12:40:09PM -0800, Jaegeuk Kim wrote:
> > > On 01/05, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > fscrypt_dummy_context_enabled() accesses ->s_cop, which now is only set
> > > > when the filesystem is built with encryption support.  This didn't
> > > > actually matter because no filesystems called it.  However, it will
> > > > start being used soon, so fix it by moving it from fscrypt.h to
> > > > fscrypt_supp.h and stubbing it out in fscrypt_notsupp.h.
> > > 
> > > Ted, do we have a chance to get rid of this dummy_context? If there exists
> > > backward compatibility issue, please never mind tho.
> > > 
> > 
> > It's used to implement the test_dummy_encryption mount option for ext4, which is
> > used by the 'ext4/encrypt' config for gce-xfstests.  Its purpose is to cause all
> > new files (directories, regular files, and symlinks) to be automatically
> > encrypted with a default key, so that the encrypted I/O paths are tested more
> > thoroughly than by just running the 'encrypt' group tests.
> > 
> > There are no backward compatibility concerns with changing or removing the
> > test_dummy_encryption mount option; we just don't have a better solution yet.
> > 
> > Ideally, instead of using test_dummy_encryption we would encrypt the root
> > directory of the filesystem immediately after it is formatted.  However, that
> > doesn't work for ext4 because the lost+found directory has to be located in the
> > root directory, and must be unencrypted, and the lost+found directory entry must
> > be unencrypted.
> > 
> > So I think getting rid of test_dummy_encryption depends on a solution to the
> > lost+found problem.
> 
> Thank you for the explanation. Actually, I've got used to encrypt the root dir
> in f2fs for such the testing purpose, since it's indeed empty. If it's the only
> matter of lost_found in ext4, how about setting the encryption bit and dummy
> context in root inode by mke2fs or tune2fs?
> 

You can't mark a nonempty directory as encrypted.  First, you can't have an
unencrypted file *name* in an encrypted directory, because then all the
operations on that filename won't work, because the filesystem will think it is
encrypted.  Second, you cannot have an unencrypted file in an encrypted
directory because that violates the constraint that all files in an encrypted
directory are encrypted -- and by design that is verified on ->lookup().

So to get it to actually work in ext4 we'd need to add extra special-case logic
around the lost+found directory -- probably allowing both the directory entry
and directory itself to be unencrypted, despite existing in an encrypted
directory.  And I think that solution would be more complicated than the
test_dummy_encryption mount option we have now.

It may be worth reconsidering if people start asking for the ability to encrypt
the root directory for other reasons, though.  (Although, that use case is
largely redundant with dm-crypt.)

Eric

Re: [PATCH v2 05/11] fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp versions

[Jaegeuk Kim]

On 01/05, Eric Biggers wrote:
> On Fri, Jan 05, 2018 at 01:36:08PM -0800, Jaegeuk Kim wrote:
> > On 01/05, Eric Biggers wrote:
> > > On Fri, Jan 05, 2018 at 12:40:09PM -0800, Jaegeuk Kim wrote:
> > > > On 01/05, Eric Biggers wrote:
> > > > > From: Eric Biggers <ebiggers@google.com>
> > > > > 
> > > > > fscrypt_dummy_context_enabled() accesses ->s_cop, which now is only set
> > > > > when the filesystem is built with encryption support.  This didn't
> > > > > actually matter because no filesystems called it.  However, it will
> > > > > start being used soon, so fix it by moving it from fscrypt.h to
> > > > > fscrypt_supp.h and stubbing it out in fscrypt_notsupp.h.
> > > > 
> > > > Ted, do we have a chance to get rid of this dummy_context? If there exists
> > > > backward compatibility issue, please never mind tho.
> > > > 
> > > 
> > > It's used to implement the test_dummy_encryption mount option for ext4, which is
> > > used by the 'ext4/encrypt' config for gce-xfstests.  Its purpose is to cause all
> > > new files (directories, regular files, and symlinks) to be automatically
> > > encrypted with a default key, so that the encrypted I/O paths are tested more
> > > thoroughly than by just running the 'encrypt' group tests.
> > > 
> > > There are no backward compatibility concerns with changing or removing the
> > > test_dummy_encryption mount option; we just don't have a better solution yet.
> > > 
> > > Ideally, instead of using test_dummy_encryption we would encrypt the root
> > > directory of the filesystem immediately after it is formatted.  However, that
> > > doesn't work for ext4 because the lost+found directory has to be located in the
> > > root directory, and must be unencrypted, and the lost+found directory entry must
> > > be unencrypted.
> > > 
> > > So I think getting rid of test_dummy_encryption depends on a solution to the
> > > lost+found problem.
> > 
> > Thank you for the explanation. Actually, I've got used to encrypt the root dir
> > in f2fs for such the testing purpose, since it's indeed empty. If it's the only
> > matter of lost_found in ext4, how about setting the encryption bit and dummy
> > context in root inode by mke2fs or tune2fs?
> > 
> 
> You can't mark a nonempty directory as encrypted.  First, you can't have an
> unencrypted file *name* in an encrypted directory, because then all the
> operations on that filename won't work, because the filesystem will think it is
> encrypted.  Second, you cannot have an unencrypted file in an encrypted
> directory because that violates the constraint that all files in an encrypted
> directory are encrypted -- and by design that is verified on ->lookup().
> 
> So to get it to actually work in ext4 we'd need to add extra special-case logic
> around the lost+found directory -- probably allowing both the directory entry
> and directory itself to be unencrypted, despite existing in an encrypted
> directory.  And I think that solution would be more complicated than the
> test_dummy_encryption mount option we have now.

Agreed that dummy'd be easy to go for now tho, doesn't it give any security
concern at all, even only for ext4 testing purpose? Is there a chance to hack
the mount option in runtime? BTW, it may be doable to build an encrypt root
inode having encrypted dentry of lost_found, given dummy_context in mke2fs.
I'm just curious whether or not it'd be worth to do.

Thanks,

> 
> It may be worth reconsidering if people start asking for the ability to encrypt
> the root directory for other reasons, though.  (Although, that use case is
> largely redundant with dm-crypt.)
> 
> Eric

Re: [PATCH v2 05/11] fscrypt: split fscrypt_dummy_context_enabled() into supp/notsupp versions

[Theodore Ts'o]

On Fri, Jan 05, 2018 at 04:39:50PM -0800, Jaegeuk Kim wrote:
> 
> Agreed that dummy'd be easy to go for now tho, doesn't it give any security
> concern at all, even only for ext4 testing purpose? Is there a chance to hack
> the mount option in runtime? BTW, it may be doable to build an encrypt root
> inode having encrypted dentry of lost_found, given dummy_context in mke2fs.
> I'm just curious whether or not it'd be worth to do.

It requires root to manipulate the mount option; and if you don't
trust root, you've got other problems.

The problem is that e2fsck needs to be able to repair a file system
without having access to any of the file system keys.  And so if there
is an orphaned inode (e.g., an inode which is not attached to any
directory), we need to put it somewhere, and that's /lost+found.

I've considered making lost+found a magic directory which isn't
directly linked into the root directory, and have a magic namei
lookup.  There are downsides though; that would mean that the
/lost+found filename has to be magic --- what to do if the user tries
to create a lost+found directory, or rename the magic lost+found
directory.

Ultimately, i decided it wasn't worth the semantic and
implementational complexity, especially since none of the primary use
cases (Android, Chromeos, etc.) needed an encrypted root directory.

      			  	       	  	    - Ted

[PATCH v2 06/11] fscrypt: move fscrypt_operations declaration to fscrypt_supp.h

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Filesystems now only define their fscrypt_operations when they are
compiled with encryption support, so move the fscrypt_operations
declaration from fscrypt.h to fscrypt_supp.h.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/fscrypt.h      | 18 ------------------
 include/linux/fscrypt_supp.h | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index b671a4eef47f..33b95a91f720 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -54,24 +54,6 @@ struct fscrypt_name {
 #define fname_name(p)		((p)->disk_name.name)
 #define fname_len(p)		((p)->disk_name.len)
 
-/*
- * fscrypt superblock flags
- */
-#define FS_CFLG_OWN_PAGES (1U << 1)
-
-/*
- * crypto opertions for filesystems
- */
-struct fscrypt_operations {
-	unsigned int flags;
-	const char *key_prefix;
-	int (*get_context)(struct inode *, void *, size_t);
-	int (*set_context)(struct inode *, const void *, size_t, void *);
-	bool (*dummy_context)(struct inode *);
-	bool (*empty_dir)(struct inode *);
-	unsigned (*max_namelen)(struct inode *);
-};
-
 /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
 #define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
 
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index e7dfa2974906..ce61caf26f40 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -11,6 +11,24 @@
 #ifndef _LINUX_FSCRYPT_SUPP_H
 #define _LINUX_FSCRYPT_SUPP_H
 
+/*
+ * fscrypt superblock flags
+ */
+#define FS_CFLG_OWN_PAGES (1U << 1)
+
+/*
+ * crypto operations for filesystems
+ */
+struct fscrypt_operations {
+	unsigned int flags;
+	const char *key_prefix;
+	int (*get_context)(struct inode *, void *, size_t);
+	int (*set_context)(struct inode *, const void *, size_t, void *);
+	bool (*dummy_context)(struct inode *);
+	bool (*empty_dir)(struct inode *);
+	unsigned (*max_namelen)(struct inode *);
+};
+
 struct fscrypt_ctx {
 	union {
 		struct {
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 07/11] fscrypt: move fscrypt_valid_enc_modes() to fscrypt_private.h

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The encryption modes are validated by fs/crypto/, not by individual
filesystems.  Therefore, move fscrypt_valid_enc_modes() from fscrypt.h
to fscrypt_private.h.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fscrypt_private.h | 14 ++++++++++++++
 include/linux/fscrypt.h     | 14 --------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 823c43a00bf7..2b848e7c92f0 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -71,6 +71,20 @@ typedef enum {
 #define FS_CTX_REQUIRES_FREE_ENCRYPT_FL		0x00000001
 #define FS_CTX_HAS_BOUNCE_BUFFER_FL		0x00000002
 
+static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
+					   u32 filenames_mode)
+{
+	if (contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
+	    filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
+		return true;
+
+	if (contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
+	    filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
+		return true;
+
+	return false;
+}
+
 /* crypto.c */
 extern struct kmem_cache *fscrypt_info_cachep;
 extern int fscrypt_initialize(unsigned int cop_flags);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 33b95a91f720..2e4dce0365cf 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -57,20 +57,6 @@ struct fscrypt_name {
 /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
 #define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
 
-static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
-					u32 filenames_mode)
-{
-	if (contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
-	    filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
-		return true;
-
-	if (contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
-	    filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
-		return true;
-
-	return false;
-}
-
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
 {
 	if (str->len == 1 && str->name[0] == '.')
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 08/11] fscrypt: move fscrypt_is_dot_dotdot() to fs/crypto/fname.c

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Only fs/crypto/fname.c cares about treating the "." and ".." filenames
specially with regards to encryption, so move fscrypt_is_dot_dotdot()
from fscrypt.h to there.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fname.c       | 11 +++++++++++
 include/linux/fscrypt.h | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 305541bcd108..b8c5061553b1 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -15,6 +15,17 @@
 #include <linux/ratelimit.h>
 #include "fscrypt_private.h"
 
+static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
+{
+	if (str->len == 1 && str->name[0] == '.')
+		return true;
+
+	if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
+		return true;
+
+	return false;
+}
+
 /**
  * fname_encrypt() - encrypt a filename
  *
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 2e4dce0365cf..3045fc49d3ca 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -57,17 +57,6 @@ struct fscrypt_name {
 /* Maximum value for the third parameter of fscrypt_operations.set_context(). */
 #define FSCRYPT_SET_CONTEXT_MAX_SIZE	28
 
-static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
-{
-	if (str->len == 1 && str->name[0] == '.')
-		return true;
-
-	if (str->len == 2 && str->name[0] == '.' && str->name[1] == '.')
-		return true;
-
-	return false;
-}
-
 #if __FS_HAS_ENCRYPTION
 #include <linux/fscrypt_supp.h>
 #else
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 09/11] fscrypt: trim down fscrypt.h includes

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

fscrypt.h included way too many other headers, given that it is included
by filesystems both with and without encryption support.  Trim down the
includes list by moving the needed includes into more appropriate
places, and removing the unneeded ones.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/crypto.c           | 1 +
 fs/crypto/fname.c            | 1 +
 fs/crypto/keyinfo.c          | 1 +
 include/linux/fscrypt.h      | 6 ------
 include/linux/fscrypt_supp.h | 3 +++
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 732a786cce9d..ce654526c0fb 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -27,6 +27,7 @@
 #include <linux/dcache.h>
 #include <linux/namei.h>
 #include <crypto/aes.h>
+#include <crypto/skcipher.h>
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index b8c5061553b1..52d4dbe1e8e7 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -13,6 +13,7 @@
 
 #include <linux/scatterlist.h>
 #include <linux/ratelimit.h>
+#include <crypto/skcipher.h>
 #include "fscrypt_private.h"
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c
index 5e6e846f5a24..c115eac4b4cf 100644
--- a/fs/crypto/keyinfo.c
+++ b/fs/crypto/keyinfo.c
@@ -14,6 +14,7 @@
 #include <linux/ratelimit.h>
 #include <crypto/aes.h>
 #include <crypto/sha.h>
+#include <crypto/skcipher.h>
 #include "fscrypt_private.h"
 
 static struct crypto_shash *essiv_hash_tfm;
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 3045fc49d3ca..071ebabfc287 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -14,13 +14,7 @@
 #ifndef _LINUX_FSCRYPT_H
 #define _LINUX_FSCRYPT_H
 
-#include <linux/key.h>
 #include <linux/fs.h>
-#include <linux/mm.h>
-#include <linux/bio.h>
-#include <linux/dcache.h>
-#include <crypto/skcipher.h>
-#include <uapi/linux/fs.h>
 
 #define FS_CRYPTO_BLOCK_SIZE		16
 
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index ce61caf26f40..562a9bc04560 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -11,6 +11,9 @@
 #ifndef _LINUX_FSCRYPT_SUPP_H
 #define _LINUX_FSCRYPT_SUPP_H
 
+#include <linux/mm.h>
+#include <linux/slab.h>
+
 /*
  * fscrypt superblock flags
  */
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 10/11] fscrypt: new helper functions for ->symlink()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently, filesystems supporting fscrypt need to implement some tricky
logic when creating encrypted symlinks, including handling a peculiar
on-disk format (struct fscrypt_symlink_data) and correctly calculating
the size of the encrypted symlink.  Introduce helper functions to make
things a bit easier:

- fscrypt_prepare_symlink() computes and validates the size the symlink
  target will require on-disk.
- fscrypt_encrypt_symlink() creates the encrypted target if needed.

The new helpers actually fix some subtle bugs.  First, when checking
whether the symlink target was too long, filesystems didn't account for
the fact that the NUL padding is meant to be truncated if it would cause
the maximum length to be exceeded, as is done for filenames in
directories.  Consequently users would receive ENAMETOOLONG when
creating symlinks close to what is supposed to be the maximum length.
For example, with EXT4 with a 4K block size, the maximum symlink target
length in an encrypted directory is supposed to be 4093 bytes (in
comparison to 4095 in an unencrypted directory), but in
FS_POLICY_FLAGS_PAD_32-mode only up to 4064 bytes were accepted.

Second, symlink targets of "." and ".." were not being encrypted, even
though they should be, as these names are special in *directory entries*
but not in symlink targets.  Fortunately, we can fix this simply by
starting to encrypt them, as old kernels already accept them in
encrypted form.

Third, the output string length the filesystems were providing when
doing the actual encryption was incorrect, as it was forgotten to
exclude 'sizeof(struct fscrypt_symlink_data)'.  Fortunately though, this
bug didn't make a difference.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/fname.c               |  8 ++--
 fs/crypto/fscrypt_private.h     |  4 ++
 fs/crypto/hooks.c               | 90 +++++++++++++++++++++++++++++++++++++++++
 include/linux/fscrypt.h         | 64 +++++++++++++++++++++++++++++
 include/linux/fscrypt_notsupp.h | 16 ++++++++
 include/linux/fscrypt_supp.h    |  6 +++
 6 files changed, 185 insertions(+), 3 deletions(-)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 52d4dbe1e8e7..62f13d533439 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -34,8 +34,8 @@ static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
  *
  * Return: 0 on success, -errno on failure
  */
-static int fname_encrypt(struct inode *inode,
-			const struct qstr *iname, struct fscrypt_str *oname)
+int fname_encrypt(struct inode *inode,
+		  const struct qstr *iname, struct fscrypt_str *oname)
 {
 	struct skcipher_request *req = NULL;
 	DECLARE_CRYPTO_WAIT(wait);
@@ -56,9 +56,11 @@ static int fname_encrypt(struct inode *inode,
 	 * Copy the filename to the output buffer for encrypting in-place and
 	 * pad it with the needed number of NUL bytes.
 	 */
+	if (WARN_ON(oname->len < iname->len))
+		return -ENOBUFS;
 	cryptlen = max_t(unsigned int, iname->len, FS_CRYPTO_BLOCK_SIZE);
 	cryptlen = round_up(cryptlen, padding);
-	cryptlen = min(cryptlen, lim);
+	cryptlen = min3(cryptlen, lim, oname->len);
 	memcpy(oname->name, iname->name, iname->len);
 	memset(oname->name + iname->len, 0, cryptlen - iname->len);
 
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 2b848e7c92f0..6995bca5006b 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -98,6 +98,10 @@ extern int fscrypt_do_page_crypto(const struct inode *inode,
 extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
 					      gfp_t gfp_flags);
 
+/* fname.c */
+extern int fname_encrypt(struct inode *inode,
+			 const struct qstr *iname, struct fscrypt_str *oname);
+
 /* keyinfo.c */
 extern void __exit fscrypt_essiv_cleanup(void);
 
diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 9f5fb2eb9cf7..4b83e4af2e41 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -110,3 +110,93 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
+
+int __fscrypt_prepare_symlink(struct inode *dir, unsigned int len,
+			      unsigned int max_len,
+			      struct fscrypt_str *disk_link)
+{
+	int err;
+
+	/*
+	 * To calculate the size of the encrypted symlink target we need to know
+	 * the amount of NUL padding, which is determined by the flags set in
+	 * the encryption policy which will be inherited from the directory.
+	 * The easiest way to get access to this is to just load the directory's
+	 * fscrypt_info, since we'll need it to create the dir_entry anyway.
+	 *
+	 * Note: in test_dummy_encryption mode, @dir may be unencrypted.
+	 */
+	err = fscrypt_get_encryption_info(dir);
+	if (err)
+		return err;
+	if (!fscrypt_has_encryption_key(dir))
+		return -ENOKEY;
+
+	/*
+	 * Calculate the size of the encrypted symlink and verify it won't
+	 * exceed max_len.  Note that for historical reasons, encrypted symlink
+	 * targets are prefixed with the ciphertext length, despite this
+	 * actually being redundant with i_size.  This decreases by 2 bytes the
+	 * longest symlink target we can accept.
+	 *
+	 * We could recover 1 byte by not counting a null terminator, but
+	 * counting it (even though it is meaningless for ciphertext) is simpler
+	 * for now since filesystems will assume it is there and subtract it.
+	 */
+	if (sizeof(struct fscrypt_symlink_data) + len > max_len)
+		return -ENAMETOOLONG;
+	disk_link->len = min_t(unsigned int,
+			       sizeof(struct fscrypt_symlink_data) +
+					fscrypt_fname_encrypted_size(dir, len),
+			       max_len);
+	disk_link->name = NULL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__fscrypt_prepare_symlink);
+
+int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
+			      unsigned int len, struct fscrypt_str *disk_link)
+{
+	int err;
+	struct qstr iname = { .name = target, .len = len };
+	struct fscrypt_symlink_data *sd;
+	unsigned int ciphertext_len;
+	struct fscrypt_str oname;
+
+	err = fscrypt_require_key(inode);
+	if (err)
+		return err;
+
+	if (disk_link->name) {
+		/* filesystem-provided buffer */
+		sd = (struct fscrypt_symlink_data *)disk_link->name;
+	} else {
+		sd = kmalloc(disk_link->len, GFP_NOFS);
+		if (!sd)
+			return -ENOMEM;
+	}
+	ciphertext_len = disk_link->len - sizeof(*sd);
+	sd->len = cpu_to_le16(ciphertext_len);
+
+	oname.name = sd->encrypted_path;
+	oname.len = ciphertext_len;
+	err = fname_encrypt(inode, &iname, &oname);
+	if (err) {
+		if (!disk_link->name)
+			kfree(sd);
+		return err;
+	}
+	BUG_ON(oname.len != ciphertext_len);
+
+	/*
+	 * Null-terminating the ciphertext doesn't make sense, but we still
+	 * count the null terminator in the length, so we might as well
+	 * initialize it just in case the filesystem writes it out.
+	 */
+	sd->encrypted_path[ciphertext_len] = '\0';
+
+	if (!disk_link->name)
+		disk_link->name = (unsigned char *)sd;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__fscrypt_encrypt_symlink);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 071ebabfc287..6a678d0e956a 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -196,4 +196,68 @@ static inline int fscrypt_prepare_setattr(struct dentry *dentry,
 	return 0;
 }
 
+/**
+ * fscrypt_prepare_symlink - prepare to create a possibly-encrypted symlink
+ * @dir: directory in which the symlink is being created
+ * @target: plaintext symlink target
+ * @len: length of @target excluding null terminator
+ * @max_len: space the filesystem has available to store the symlink target
+ * @disk_link: (out) the on-disk symlink target being prepared
+ *
+ * This function computes the size the symlink target will require on-disk,
+ * stores it in @disk_link->len, and validates it against @max_len.  An
+ * encrypted symlink may be longer than the original.
+ *
+ * Additionally, @disk_link->name is set to @target if the symlink will be
+ * unencrypted, but left NULL if the symlink will be encrypted.  For encrypted
+ * symlinks, the filesystem must call fscrypt_encrypt_symlink() to create the
+ * on-disk target later.  (The reason for the two-step process is that some
+ * filesystems need to know the size of the symlink target before creating the
+ * inode, e.g. to determine whether it will be a "fast" or "slow" symlink.)
+ *
+ * Return: 0 on success, -ENAMETOOLONG if the symlink target is too long,
+ * -ENOKEY if the encryption key is missing, or another -errno code if a problem
+ * occurred while setting up the encryption key.
+ */
+static inline int fscrypt_prepare_symlink(struct inode *dir,
+					  const char *target,
+					  unsigned int len,
+					  unsigned int max_len,
+					  struct fscrypt_str *disk_link)
+{
+	if (IS_ENCRYPTED(dir) || fscrypt_dummy_context_enabled(dir))
+		return __fscrypt_prepare_symlink(dir, len, max_len, disk_link);
+
+	disk_link->name = (unsigned char *)target;
+	disk_link->len = len + 1;
+	if (disk_link->len > max_len)
+		return -ENAMETOOLONG;
+	return 0;
+}
+
+/**
+ * fscrypt_encrypt_symlink - encrypt the symlink target if needed
+ * @inode: symlink inode
+ * @target: plaintext symlink target
+ * @len: length of @target excluding null terminator
+ * @disk_link: (in/out) the on-disk symlink target being prepared
+ *
+ * If the symlink target needs to be encrypted, then this function encrypts it
+ * into @disk_link->name.  fscrypt_prepare_symlink() must have been called
+ * previously to compute @disk_link->len.  If the filesystem did not allocate a
+ * buffer for @disk_link->name after calling fscrypt_prepare_link(), then one
+ * will be kmalloc()'ed and the filesystem will be responsible for freeing it.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+static inline int fscrypt_encrypt_symlink(struct inode *inode,
+					  const char *target,
+					  unsigned int len,
+					  struct fscrypt_str *disk_link)
+{
+	if (IS_ENCRYPTED(inode))
+		return __fscrypt_encrypt_symlink(inode, target, len, disk_link);
+	return 0;
+}
+
 #endif	/* _LINUX_FSCRYPT_H */
diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h
index 81e02201b215..02ec0aa894d8 100644
--- a/include/linux/fscrypt_notsupp.h
+++ b/include/linux/fscrypt_notsupp.h
@@ -223,4 +223,20 @@ static inline int __fscrypt_prepare_lookup(struct inode *dir,
 	return -EOPNOTSUPP;
 }
 
+static inline int __fscrypt_prepare_symlink(struct inode *dir,
+					    unsigned int len,
+					    unsigned int max_len,
+					    struct fscrypt_str *disk_link)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int __fscrypt_encrypt_symlink(struct inode *inode,
+					    const char *target,
+					    unsigned int len,
+					    struct fscrypt_str *disk_link)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif	/* _LINUX_FSCRYPT_NOTSUPP_H */
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index 562a9bc04560..7e0b67ccd816 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -205,5 +205,11 @@ extern int __fscrypt_prepare_rename(struct inode *old_dir,
 				    struct dentry *new_dentry,
 				    unsigned int flags);
 extern int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry);
+extern int __fscrypt_prepare_symlink(struct inode *dir, unsigned int len,
+				     unsigned int max_len,
+				     struct fscrypt_str *disk_link);
+extern int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
+				     unsigned int len,
+				     struct fscrypt_str *disk_link);
 
 #endif	/* _LINUX_FSCRYPT_SUPP_H */
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCH v2 11/11] fscrypt: new helper function - fscrypt_get_symlink()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Filesystems also have duplicate code to support ->get_link() on
encrypted symlinks.  Factor it out into a new function
fscrypt_get_symlink().  It takes in the contents of the encrypted
symlink on-disk and provides the target (decrypted or encoded) that
should be returned from ->get_link().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/hooks.c               | 73 +++++++++++++++++++++++++++++++++++++++++
 include/linux/fscrypt_notsupp.h |  8 +++++
 include/linux/fscrypt_supp.h    |  3 ++
 3 files changed, 84 insertions(+)

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 4b83e4af2e41..8900e348ba6e 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -200,3 +200,76 @@ int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__fscrypt_encrypt_symlink);
+
+/**
+ * fscrypt_get_symlink - get the target of an encrypted symlink
+ * @inode: the symlink inode
+ * @caddr: the on-disk contents of the symlink
+ * @max_size: size of @caddr buffer
+ * @done: if successful, will be set up to free the returned target
+ *
+ * If the symlink's encryption key is available, we decrypt its target.
+ * Otherwise, we encode its target for presentation.
+ *
+ * This may sleep, so the filesystem must have dropped out of RCU mode already.
+ *
+ * Return: the presentable symlink target or an ERR_PTR()
+ */
+const char *fscrypt_get_symlink(struct inode *inode, const void *caddr,
+				unsigned int max_size,
+				struct delayed_call *done)
+{
+	const struct fscrypt_symlink_data *sd;
+	struct fscrypt_str cstr, pstr;
+	int err;
+
+	/* This is for encrypted symlinks only */
+	if (WARN_ON(!IS_ENCRYPTED(inode)))
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * Try to set up the symlink's encryption key, but we can continue
+	 * regardless of whether the key is available or not.
+	 */
+	err = fscrypt_get_encryption_info(inode);
+	if (err)
+		return ERR_PTR(err);
+
+	/*
+	 * For historical reasons, encrypted symlink targets are prefixed with
+	 * the ciphertext length, even though this is redundant with i_size.
+	 */
+
+	if (max_size < sizeof(*sd))
+		return ERR_PTR(-EUCLEAN);
+	sd = caddr;
+	cstr.name = (unsigned char *)sd->encrypted_path;
+	cstr.len = le16_to_cpu(sd->len);
+
+	if (cstr.len == 0)
+		return ERR_PTR(-EUCLEAN);
+
+	if (cstr.len + sizeof(*sd) - 1 > max_size)
+		return ERR_PTR(-EUCLEAN);
+
+	err = fscrypt_fname_alloc_buffer(inode, cstr.len, &pstr);
+	if (err)
+		return ERR_PTR(err);
+
+	err = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
+	if (err)
+		goto err_kfree;
+
+	err = -EUCLEAN;
+	if (pstr.name[0] == '\0')
+		goto err_kfree;
+
+	pstr.name[pstr.len] = '\0';
+	set_delayed_call(done, kfree_link, pstr.name);
+	return pstr.name;
+
+err_kfree:
+	kfree(pstr.name);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(fscrypt_get_symlink);
diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h
index 02ec0aa894d8..dd106640c6ea 100644
--- a/include/linux/fscrypt_notsupp.h
+++ b/include/linux/fscrypt_notsupp.h
@@ -239,4 +239,12 @@ static inline int __fscrypt_encrypt_symlink(struct inode *inode,
 	return -EOPNOTSUPP;
 }
 
+static inline const char *fscrypt_get_symlink(struct inode *inode,
+					      const void *caddr,
+					      unsigned int max_size,
+					      struct delayed_call *done)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif	/* _LINUX_FSCRYPT_NOTSUPP_H */
diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h
index 7e0b67ccd816..dc2babf3f7d3 100644
--- a/include/linux/fscrypt_supp.h
+++ b/include/linux/fscrypt_supp.h
@@ -211,5 +211,8 @@ extern int __fscrypt_prepare_symlink(struct inode *dir, unsigned int len,
 extern int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
 				     unsigned int len,
 				     struct fscrypt_str *disk_link);
+extern const char *fscrypt_get_symlink(struct inode *inode, const void *caddr,
+				       unsigned int max_size,
+				       struct delayed_call *done);
 
 #endif	/* _LINUX_FSCRYPT_SUPP_H */
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCH v2 00/11] fscrypt: symlink helpers and fscrypt.h cleanup

[Theodore Ts'o]

I've taken the v2 version of this series, plus the ext4-specific
patches to use the new symlink helpers (from the v1 patch series).
After testing and validating them, I included the patches to convert
f2fs and ubifs to use the symlink helpers, so I could also grab the
cleanup patches from the V1 series.

Jaeguk, Richard, please let me know if you are OK with my taking the
f2fs and ubifs changes in the fscrypt tree.  It seems like it would
keep things simpler if we did that.

Cheers,

						- Ted

Re: [PATCH v2 00/11] fscrypt: symlink helpers and fscrypt.h cleanup

[Jaegeuk Kim]

On 01/11, Theodore Ts'o wrote:
> I've taken the v2 version of this series, plus the ext4-specific
> patches to use the new symlink helpers (from the v1 patch series).
> After testing and validating them, I included the patches to convert
> f2fs and ubifs to use the symlink helpers, so I could also grab the
> cleanup patches from the V1 series.
> 
> Jaeguk, Richard, please let me know if you are OK with my taking the
> f2fs and ubifs changes in the fscrypt tree.  It seems like it would
> keep things simpler if we did that.

I'm fine with that.
Thanks,

> 
> Cheers,
> 
> 						- Ted