[PATCH] ext4 crypto: handle ENOKEY correctly

[PATCH] ext4 crypto: handle ENOKEY correctly

[Dmitry Monakhov]

Currently we try to hide ENOKEY inside ext4_get_encryption_info(), but
it is not always correct. There are two class of callers ext4_get_encryption_info()
1) The one where we can ignore ENOKEY
    - ext4_setup_fname_crypto()
    - ext4_is_child_context_consistent_with_parent()
2) The one do care about any error because expect that ei->i_crypt_info will
   be initalized after ext4_get_encryption_info() succeed
   - ext4_file_mmap
   - ext4_file_open
   - ext4_inherit_context (key may becomes obsoleted, revoked, dead any time)

So let's return ENOKEY from ext4_get_encryption_info() if necessery and let caller
handle it correctly.

#Test case (try to read encrypted file w/o key)
keyctl clear @s
mount $DEV /mnt
cat > /mnt/enc_dir/enc_file
#Result
kernel BUG at fs/ext4/crypto.c:109
Call Trace:
 [<ffffffff81251311>] ext4_mpage_readpages+0x3ce/0x55d
 [<ffffffff810b6ee4>] ? sched_clock_cpu+0x8c/0xa8
 [<ffffffff81214f7d>] ext4_readpages+0x3c/0x3e
 [<ffffffff8115ac52>] __do_page_cache_readahead+0x164/0x1e6
 [<ffffffff8115aec3>] ondemand_readahead+0x1ef/0x204
 [<ffffffff8115b0d1>] page_cache_sync_readahead+0x40/0x42
 [<ffffffff81152d5d>] generic_file_read_iter+0x1b3/0x50d
 [<ffffffff8119070e>] __vfs_read+0xb3/0xd2
 [<ffffffff811918b8>] vfs_read+0x8f/0xcf
 [<ffffffff8119031a>] ? fdget_pos+0xd/0x18
 [<ffffffff811919e0>] SyS_read+0x5c/0x8c
 [<ffffffff817125ae>] system_call_fastpath+0x12/0x76

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/crypto_fname.c  |    2 +-
 fs/ext4/crypto_key.c    |    2 --
 fs/ext4/crypto_policy.c |    4 ++--
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/crypto_fname.c b/fs/ext4/crypto_fname.c
index e63dd29..9f450d3 100644
--- a/fs/ext4/crypto_fname.c
+++ b/fs/ext4/crypto_fname.c
@@ -265,7 +265,7 @@ int ext4_setup_fname_crypto(struct inode *inode)
 		return 0;
 
 	res = ext4_get_encryption_info(inode);
-	if (res < 0)
+	if (res < 0 && res != -ENOKEY)
 		return res;
 	ci = ei->i_crypt_info;
 
diff --git a/fs/ext4/crypto_key.c b/fs/ext4/crypto_key.c
index 858d7d6..db31972 100644
--- a/fs/ext4/crypto_key.c
+++ b/fs/ext4/crypto_key.c
@@ -191,8 +191,6 @@ int _ext4_get_encryption_info(struct inode *inode)
 				  crypt_info->ci_raw);
 out:
 	if (res < 0) {
-		if (res == -ENOKEY)
-			res = 0;
 		kmem_cache_free(ext4_crypt_info_cachep, crypt_info);
 	} else {
 		ei->i_crypt_info = crypt_info;
diff --git a/fs/ext4/crypto_policy.c b/fs/ext4/crypto_policy.c
index 683391f..db6e9f8 100644
--- a/fs/ext4/crypto_policy.c
+++ b/fs/ext4/crypto_policy.c
@@ -144,10 +144,10 @@ int ext4_is_child_context_consistent_with_parent(struct inode *parent,
 	if (!ext4_encrypted_inode(child))
 		return 0;
 	res = ext4_get_encryption_info(parent);
-	if (res)
+	if (res && res != -ENOKEY)
 		return 0;
 	res = ext4_get_encryption_info(child);
-	if (res)
+	if (res && res != -ENOKEY)
 		return 0;
 	parent_ci = EXT4_I(parent)->i_crypt_info;
 	child_ci = EXT4_I(child)->i_crypt_info;
-- 
1.7.1

Re: [PATCH] ext4 crypto: handle ENOKEY correctly

[Theodore Ts'o]

On Fri, May 29, 2015 at 08:39:03PM +0400, Dmitry Monakhov wrote:
> Currently we try to hide ENOKEY inside ext4_get_encryption_info(), but
> it is not always correct. There are two class of callers ext4_get_encryption_info()
> 1) The one where we can ignore ENOKEY
>     - ext4_setup_fname_crypto()
>     - ext4_is_child_context_consistent_with_parent()
> 2) The one do care about any error because expect that ei->i_crypt_info will
>    be initalized after ext4_get_encryption_info() succeed
>    - ext4_file_mmap
>    - ext4_file_open
>    - ext4_inherit_context (key may becomes obsoleted, revoked, dead any time)
> 
> So let's return ENOKEY from ext4_get_encryption_info() if necessery and let caller
> handle it correctly.

I don't think that's the right way to go.  We should add checks to
ext4_file_open, sure.  But the problem is that i_crypt_info can get
set to NULL after the file is succesfully opened.  So we need to
handle i_crypt_info being NULL everywhere.  So the BUG_ON() in
ext4_get_crypto_ctx() needs to be replaced with:

	if (ci == NULL)
		return ERR_PTR(-ENOKEY);

I'll also add documentation making it clear that having
ext4_get_encryption_info() returning with i_crypt_info set to NULL is
considered a successful return, and that callers like
ext4_file_mmap(), ext4_file_open(), ext4_inherit_context(),
needs to check if i_crypt_info is NULL instead.

      	       	  	       	       - Ted

Re: [PATCH] ext4 crypto: handle ENOKEY correctly

[Theodore Ts'o]

On Fri, May 29, 2015 at 04:44:29PM -0400, Theodore Ts'o wrote:
> I don't think that's the right way to go.  We should add checks to
> ext4_file_open, sure.  But the problem is that i_crypt_info can get
> set to NULL after the file is succesfully opened.  So we need to
> handle i_crypt_info being NULL everywhere.  So the BUG_ON() in
> ext4_get_crypto_ctx() needs to be replaced with:
> 
> 	if (ci == NULL)
> 		return ERR_PTR(-ENOKEY);

This is what I had in mind....

				- Ted

commit 3cde0c5d3125697c4b02c32054a378dc71ebfdb5
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Sun May 31 08:12:34 2015 -0400

    ext4 crypto: handle unexpected lack of encryption keys
    
    Fix up attempts by users to try to write to a file when they don't
    have access to the encryption key.
    
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index ac2419c..6634478 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -104,7 +104,8 @@ struct ext4_crypto_ctx *ext4_get_crypto_ctx(struct inode *inode)
 	unsigned long flags;
 	struct ext4_crypt_info *ci = EXT4_I(inode)->i_crypt_info;
 
-	BUG_ON(ci == NULL);
+	if (ci == NULL)
+		return ERR_PTR(-ENOKEY);
 
 	/*
 	 * We first try getting the ctx from a free list because in
diff --git a/fs/ext4/crypto_policy.c b/fs/ext4/crypto_policy.c
index a1d434d..02c4e5d 100644
--- a/fs/ext4/crypto_policy.c
+++ b/fs/ext4/crypto_policy.c
@@ -183,7 +183,8 @@ int ext4_inherit_context(struct inode *parent, struct inode *child)
 	if (res < 0)
 		return res;
 	ci = EXT4_I(parent)->i_crypt_info;
-	BUG_ON(ci == NULL);
+	if (ci == NULL)
+		return -ENOKEY;
 
 	ctx.format = EXT4_ENCRYPTION_CONTEXT_FORMAT_V1;
 	if (DUMMY_ENCRYPTION_ENABLED(EXT4_SB(parent->i_sb))) {
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 875ca6b..ac517f1 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -226,6 +226,8 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
 		int err = ext4_get_encryption_info(inode);
 		if (err)
 			return 0;
+		if (ext4_encryption_info(inode) == NULL)
+			return -ENOKEY;
 	}
 	file_accessed(file);
 	if (IS_DAX(file_inode(file))) {
@@ -278,6 +280,13 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 			ext4_journal_stop(handle);
 		}
 	}
+	if (ext4_encrypted_inode(inode)) {
+		ret = ext4_get_encryption_info(inode);
+		if (ret)
+			return -EACCES;
+		if (ext4_encryption_info(inode) == NULL)
+			return -ENOKEY;
+	}
 	/*
 	 * Set up the jbd2_inode if we are opening the inode for
 	 * writing and the journal is present
@@ -287,13 +296,7 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 		if (ret < 0)
 			return ret;
 	}
-	ret = dquot_file_open(inode, filp);
-	if (!ret && ext4_encrypted_inode(inode)) {
-		ret = ext4_get_encryption_info(inode);
-		if (ret)
-			ret = -EACCES;
-	}
-	return ret;
+	return dquot_file_open(inode, filp);
 }
 
 /*

Re: [PATCH] ext4 crypto: handle ENOKEY correctly

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 3797 bytes --]

Theodore Ts'o <tytso@mit.edu> writes:

> On Fri, May 29, 2015 at 04:44:29PM -0400, Theodore Ts'o wrote:
>> I don't think that's the right way to go.  We should add checks to
>> ext4_file_open, sure.  But the problem is that i_crypt_info can get
>> set to NULL after the file is succesfully opened.  So we need to
>> handle i_crypt_info being NULL everywhere.  So the BUG_ON() in
>> ext4_get_crypto_ctx() needs to be replaced with:
>> 
>> 	if (ci == NULL)
>> 		return ERR_PTR(-ENOKEY);
>
> This is what I had in mind....
ACK. with one note, you forget to convert all callers of
ext4_get_crypto_ctx() to new error convention. Please see patch below.

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index a9fd028..6f9d95e 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -467,8 +467,9 @@ int ext4_decrypt_one(struct inode *inode, struct page *page)
 
 	struct ext4_crypto_ctx *ctx = ext4_get_crypto_ctx(inode);
 
-	if (!ctx)
-		return -ENOMEM;
+	if (IS_ERR(ctx))
+		return PTR_ERR(ctx);
+
 	ret = ext4_decrypt(ctx, page);
 	ext4_release_crypto_ctx(ctx);
 	return ret;

>
> 				- Ted
>
> commit 3cde0c5d3125697c4b02c32054a378dc71ebfdb5
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Sun May 31 08:12:34 2015 -0400
>
>     ext4 crypto: handle unexpected lack of encryption keys
>     
>     Fix up attempts by users to try to write to a file when they don't
>     have access to the encryption key.
>     
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
>
> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> index ac2419c..6634478 100644
> --- a/fs/ext4/crypto.c
> +++ b/fs/ext4/crypto.c
> @@ -104,7 +104,8 @@ struct ext4_crypto_ctx *ext4_get_crypto_ctx(struct inode *inode)
>  	unsigned long flags;
>  	struct ext4_crypt_info *ci = EXT4_I(inode)->i_crypt_info;
>  
> -	BUG_ON(ci == NULL);
> +	if (ci == NULL)
> +		return ERR_PTR(-ENOKEY);
>  
>  	/*
>  	 * We first try getting the ctx from a free list because in
> diff --git a/fs/ext4/crypto_policy.c b/fs/ext4/crypto_policy.c
> index a1d434d..02c4e5d 100644
> --- a/fs/ext4/crypto_policy.c
> +++ b/fs/ext4/crypto_policy.c
> @@ -183,7 +183,8 @@ int ext4_inherit_context(struct inode *parent, struct inode *child)
>  	if (res < 0)
>  		return res;
>  	ci = EXT4_I(parent)->i_crypt_info;
> -	BUG_ON(ci == NULL);
> +	if (ci == NULL)
> +		return -ENOKEY;
>  
>  	ctx.format = EXT4_ENCRYPTION_CONTEXT_FORMAT_V1;
>  	if (DUMMY_ENCRYPTION_ENABLED(EXT4_SB(parent->i_sb))) {
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 875ca6b..ac517f1 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -226,6 +226,8 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
>  		int err = ext4_get_encryption_info(inode);
>  		if (err)
>  			return 0;
> +		if (ext4_encryption_info(inode) == NULL)
> +			return -ENOKEY;
>  	}
>  	file_accessed(file);
>  	if (IS_DAX(file_inode(file))) {
> @@ -278,6 +280,13 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
>  			ext4_journal_stop(handle);
>  		}
>  	}
> +	if (ext4_encrypted_inode(inode)) {
> +		ret = ext4_get_encryption_info(inode);
> +		if (ret)
> +			return -EACCES;
> +		if (ext4_encryption_info(inode) == NULL)
> +			return -ENOKEY;
> +	}
>  	/*
>  	 * Set up the jbd2_inode if we are opening the inode for
>  	 * writing and the journal is present
> @@ -287,13 +296,7 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
>  		if (ret < 0)
>  			return ret;
>  	}
> -	ret = dquot_file_open(inode, filp);
> -	if (!ret && ext4_encrypted_inode(inode)) {
> -		ret = ext4_get_encryption_info(inode);
> -		if (ret)
> -			ret = -EACCES;
> -	}
> -	return ret;
> +	return dquot_file_open(inode, filp);
>  }
>  
>  /*

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

Re: [PATCH] ext4 crypto: handle ENOKEY correctly

[Theodore Ts'o]

On Mon, Jun 01, 2015 at 12:59:10PM +0300, Dmitry Monakhov wrote:
> Theodore Ts'o <tytso@mit.edu> writes:
> 
> > On Fri, May 29, 2015 at 04:44:29PM -0400, Theodore Ts'o wrote:
> >> I don't think that's the right way to go.  We should add checks to
> >> ext4_file_open, sure.  But the problem is that i_crypt_info can get
> >> set to NULL after the file is succesfully opened.  So we need to
> >> handle i_crypt_info being NULL everywhere.  So the BUG_ON() in
> >> ext4_get_crypto_ctx() needs to be replaced with:
> >> 
> >> 	if (ci == NULL)
> >> 		return ERR_PTR(-ENOKEY);
> >
> > This is what I had in mind....
> ACK. with one note, you forget to convert all callers of
> ext4_get_crypto_ctx() to new error convention. Please see patch below.

Thanks for pointing that out!  Fixed.

					- Ted