diff mbox

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

Message ID 20180105184502.48473-6-ebiggers3@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Biggers Jan. 5, 2018, 6:44 p.m. UTC
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(-)

Comments

Jaegeuk Kim Jan. 5, 2018, 8:40 p.m. UTC | #1
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
Eric Biggers Jan. 5, 2018, 9:18 p.m. UTC | #2
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
Jaegeuk Kim Jan. 5, 2018, 9:36 p.m. UTC | #3
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
Eric Biggers Jan. 5, 2018, 10:01 p.m. UTC | #4
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
Jaegeuk Kim Jan. 6, 2018, 12:39 a.m. UTC | #5
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
Theodore Ts'o Jan. 6, 2018, 2:49 a.m. UTC | #6
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
diff mbox

Patch

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 *);