[PATCH 1/2] eCryptfs: ecryptfs_keyring_auth_tok_for_sig() bug fix

[PATCH 1/2] eCryptfs: ecryptfs_keyring_auth_tok_for_sig() bug fix

[Roberto Sassu]

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

The pointer '(*auth_tok_key)' must be set to NULL if the requested key
cannot be retrieved in order to prevent the use by calling functions.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
---
 fs/ecryptfs/keystore.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
index c1436cf..4feb78c 100644
--- a/fs/ecryptfs/keystore.c
+++ b/fs/ecryptfs/keystore.c
@@ -1563,6 +1563,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
 		printk(KERN_ERR "Could not find key with description: [%s]\n",
 		       sig);
 		rc = process_request_key_err(PTR_ERR(*auth_tok_key));
+		(*auth_tok_key) = NULL;
 		goto out;
 	}
 	(*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
-- 
1.7.3.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

[PATCH 2/2] eCryptfs: lock requested keys for writing

[Roberto Sassu]

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

Keys requested by eCryptfs are exclusively locked in order to prevent
modifications by other subjects. In particular those which description is
specified at mount time are locked until the filesystem is unmounted, these
required to decrypt a single file are unlocked immediately after the open.

Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
Reported-by: David Howells <dhowells@redhat.com>
---
 fs/ecryptfs/crypto.c   |    4 +++-
 fs/ecryptfs/keystore.c |   15 ++++++++++++---
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index bfd8b68..6b32776 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -268,8 +268,10 @@ void ecryptfs_destroy_mount_crypt_stat(
 		list_del(&auth_tok->mount_crypt_stat_list);
 		mount_crypt_stat->num_global_auth_toks--;
 		if (auth_tok->global_auth_tok_key
-		    && !(auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID))
+		    && !(auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID)) {
+			up_write(&(auth_tok->global_auth_tok_key->sem));
 			key_put(auth_tok->global_auth_tok_key);
+		}
 		kmem_cache_free(ecryptfs_global_auth_tok_cache, auth_tok);
 	}
 	mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex);
diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
index 4feb78c..c90456a 100644
--- a/fs/ecryptfs/keystore.c
+++ b/fs/ecryptfs/keystore.c
@@ -765,8 +765,10 @@ out_free_unlock:
 out_unlock:
 	mutex_unlock(s->tfm_mutex);
 out:
-	if (auth_tok_key)
+	if (auth_tok_key) {
+		up_write(&(auth_tok_key->sem));
 		key_put(auth_tok_key);
+	}
 	kfree(s);
 	return rc;
 }
@@ -1002,8 +1004,10 @@ out:
 		(*filename_size) = 0;
 		(*filename) = NULL;
 	}
-	if (auth_tok_key)
+	if (auth_tok_key) {
+		up_write(&(auth_tok_key->sem));
 		key_put(auth_tok_key);
+	}
 	kfree(s);
 	return rc;
 }
@@ -1566,6 +1570,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
 		(*auth_tok_key) = NULL;
 		goto out;
 	}
+	down_write(&((*auth_tok_key)->sem));
 	(*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
 	if (ecryptfs_verify_version((*auth_tok)->version)) {
 		printk(KERN_ERR
@@ -1587,6 +1592,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
 	}
 out_release_key:
 	if (rc) {
+		up_write(&((*auth_tok_key)->sem));
 		key_put(*auth_tok_key);
 		(*auth_tok_key) = NULL;
 	}
@@ -1810,6 +1816,7 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
 find_next_matching_auth_tok:
 	found_auth_tok = 0;
 	if (auth_tok_key) {
+		up_write(&(auth_tok_key->sem));
 		key_put(auth_tok_key);
 		auth_tok_key = NULL;
 	}
@@ -1896,8 +1903,10 @@ found_matching_auth_tok:
 out_wipe_list:
 	wipe_auth_tok_list(&auth_tok_list);
 out:
-	if (auth_tok_key)
+	if (auth_tok_key) {
+		up_write(&(auth_tok_key->sem));
 		key_put(auth_tok_key);
+	}
 	return rc;
 }
 
-- 
1.7.3.4


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 2061 bytes --]

Re: [PATCH 2/2] eCryptfs: lock requested keys for writing

[Tyler Hicks]

On Fri Jan 28, 2011 at 11:42:04AM +0100, Roberto Sassu <roberto.sassu@polito.it> wrote:
> Keys requested by eCryptfs are exclusively locked in order to prevent
> modifications by other subjects. In particular those which description is
> specified at mount time are locked until the filesystem is unmounted, these

I don't think the approach taken for the mount wide keys will work for a
couple reasons. The first is that lockdep reports this after a mount:

================================================
[ BUG: lock held when returning to user space! ]
------------------------------------------------
mount/1019 is leaving the kernel with locks still held!
1 lock held by mount/1019:
#0:  (&key->sem){+.+.+.}, at: [<ffffffffa021ff12>] ecryptfs_keyring_auth_tok_for_sig+0xe6/0x195 [ecryptfs]

The second is that while this does prevent key_update() from updating
the key underneath us, it just results in `keyctl update` on a mount key
to become a hung task until unmounting the eCryptfs mount.

It looks like we'll have to have more efficient locking for mount keys.

Tyler

> required to decrypt a single file are unlocked immediately after the open.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@polito.it>
> Reported-by: David Howells <dhowells@redhat.com>
> ---
>  fs/ecryptfs/crypto.c   |    4 +++-
>  fs/ecryptfs/keystore.c |   15 ++++++++++++---
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index bfd8b68..6b32776 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -268,8 +268,10 @@ void ecryptfs_destroy_mount_crypt_stat(
>  		list_del(&auth_tok->mount_crypt_stat_list);
>  		mount_crypt_stat->num_global_auth_toks--;
>  		if (auth_tok->global_auth_tok_key
> -		    && !(auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID))
> +		    && !(auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID)) {
> +			up_write(&(auth_tok->global_auth_tok_key->sem));
>  			key_put(auth_tok->global_auth_tok_key);
> +		}
>  		kmem_cache_free(ecryptfs_global_auth_tok_cache, auth_tok);
>  	}
>  	mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex);
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index 4feb78c..c90456a 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -765,8 +765,10 @@ out_free_unlock:
>  out_unlock:
>  	mutex_unlock(s->tfm_mutex);
>  out:
> -	if (auth_tok_key)
> +	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
> +	}
>  	kfree(s);
>  	return rc;
>  }
> @@ -1002,8 +1004,10 @@ out:
>  		(*filename_size) = 0;
>  		(*filename) = NULL;
>  	}
> -	if (auth_tok_key)
> +	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
> +	}
>  	kfree(s);
>  	return rc;
>  }
> @@ -1566,6 +1570,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
>  		(*auth_tok_key) = NULL;
>  		goto out;
>  	}
> +	down_write(&((*auth_tok_key)->sem));
>  	(*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
>  	if (ecryptfs_verify_version((*auth_tok)->version)) {
>  		printk(KERN_ERR
> @@ -1587,6 +1592,7 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
>  	}
>  out_release_key:
>  	if (rc) {
> +		up_write(&((*auth_tok_key)->sem));
>  		key_put(*auth_tok_key);
>  		(*auth_tok_key) = NULL;
>  	}
> @@ -1810,6 +1816,7 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
>  find_next_matching_auth_tok:
>  	found_auth_tok = 0;
>  	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
>  		auth_tok_key = NULL;
>  	}
> @@ -1896,8 +1903,10 @@ found_matching_auth_tok:
>  out_wipe_list:
>  	wipe_auth_tok_list(&auth_tok_list);
>  out:
> -	if (auth_tok_key)
> +	if (auth_tok_key) {
> +		up_write(&(auth_tok_key->sem));
>  		key_put(auth_tok_key);
> +	}
>  	return rc;
>  }
> 
> -- 
> 1.7.3.4
>