* [PATCH] ecryptfs: avoid ctx initialization race
@ 2013-08-13 22:02 Kees Cook
2013-09-07 0:30 ` Tyler Hicks
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2013-08-13 22:02 UTC (permalink / raw)
To: linux-kernel; +Cc: Tyler Hicks, ecryptfs
It might be possible for two callers to race the mutex lock after the
NULL ctx check. Instead, move the lock above the check so there isn't
the possibility of leaking a crypto ctx. Additionally, report the full
algo name when failing.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/ecryptfs/crypto.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index d107576..c134346 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -618,27 +618,28 @@ int ecryptfs_init_crypt_ctx(struct ecryptfs_crypt_stat *crypt_stat)
"key_size_bits = [%zd]\n",
crypt_stat->cipher, (int)strlen(crypt_stat->cipher),
crypt_stat->key_size << 3);
+ mutex_lock(&crypt_stat->cs_tfm_mutex);
if (crypt_stat->tfm) {
rc = 0;
- goto out;
+ goto out_unlock;
}
- mutex_lock(&crypt_stat->cs_tfm_mutex);
rc = ecryptfs_crypto_api_algify_cipher_name(&full_alg_name,
crypt_stat->cipher, "cbc");
if (rc)
goto out_unlock;
crypt_stat->tfm = crypto_alloc_ablkcipher(full_alg_name, 0, 0);
- kfree(full_alg_name);
if (IS_ERR(crypt_stat->tfm)) {
rc = PTR_ERR(crypt_stat->tfm);
crypt_stat->tfm = NULL;
ecryptfs_printk(KERN_ERR, "cryptfs: init_crypt_ctx(): "
"Error initializing cipher [%s]\n",
- crypt_stat->cipher);
- goto out_unlock;
+ full_alg_name);
+ goto out_free;
}
crypto_ablkcipher_set_flags(crypt_stat->tfm, CRYPTO_TFM_REQ_WEAK_KEY);
rc = 0;
+out_free:
+ kfree(full_alg_name);
out_unlock:
mutex_unlock(&crypt_stat->cs_tfm_mutex);
out:
--
1.7.9.5
--
Kees Cook
Chrome OS Security
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ecryptfs: avoid ctx initialization race
2013-08-13 22:02 [PATCH] ecryptfs: avoid ctx initialization race Kees Cook
@ 2013-09-07 0:30 ` Tyler Hicks
2013-09-07 0:46 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: Tyler Hicks @ 2013-09-07 0:30 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-kernel, ecryptfs
[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]
On 2013-08-13 15:02:27, Kees Cook wrote:
> It might be possible for two callers to race the mutex lock after the
> NULL ctx check. Instead, move the lock above the check so there isn't
> the possibility of leaking a crypto ctx. Additionally, report the full
> algo name when failing.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Thanks, Kees!
I've pushed this to my next branch and it'll be included in a pull
request early next week.
I made one small change to this patch. See below.
> ---
> fs/ecryptfs/crypto.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index d107576..c134346 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -618,27 +618,28 @@ int ecryptfs_init_crypt_ctx(struct ecryptfs_crypt_stat *crypt_stat)
> "key_size_bits = [%zd]\n",
> crypt_stat->cipher, (int)strlen(crypt_stat->cipher),
> crypt_stat->key_size << 3);
> + mutex_lock(&crypt_stat->cs_tfm_mutex);
> if (crypt_stat->tfm) {
> rc = 0;
> - goto out;
> + goto out_unlock;
> }
> - mutex_lock(&crypt_stat->cs_tfm_mutex);
> rc = ecryptfs_crypto_api_algify_cipher_name(&full_alg_name,
> crypt_stat->cipher, "cbc");
> if (rc)
> goto out_unlock;
> crypt_stat->tfm = crypto_alloc_ablkcipher(full_alg_name, 0, 0);
> - kfree(full_alg_name);
> if (IS_ERR(crypt_stat->tfm)) {
> rc = PTR_ERR(crypt_stat->tfm);
> crypt_stat->tfm = NULL;
> ecryptfs_printk(KERN_ERR, "cryptfs: init_crypt_ctx(): "
> "Error initializing cipher [%s]\n",
> - crypt_stat->cipher);
> - goto out_unlock;
> + full_alg_name);
> + goto out_free;
> }
> crypto_ablkcipher_set_flags(crypt_stat->tfm, CRYPTO_TFM_REQ_WEAK_KEY);
> rc = 0;
> +out_free:
> + kfree(full_alg_name);
> out_unlock:
> mutex_unlock(&crypt_stat->cs_tfm_mutex);
> out:
The out label is no longer used. I removed it when I committed this
patch.
Tyler
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ecryptfs: avoid ctx initialization race
2013-09-07 0:30 ` Tyler Hicks
@ 2013-09-07 0:46 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2013-09-07 0:46 UTC (permalink / raw)
To: Tyler Hicks; +Cc: LKML, ecryptfs
On Fri, Sep 6, 2013 at 5:30 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 2013-08-13 15:02:27, Kees Cook wrote:
>> It might be possible for two callers to race the mutex lock after the
>> NULL ctx check. Instead, move the lock above the check so there isn't
>> the possibility of leaking a crypto ctx. Additionally, report the full
>> algo name when failing.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Thanks, Kees!
>
> I've pushed this to my next branch and it'll be included in a pull
> request early next week.
>
> I made one small change to this patch. See below.
>
>> ---
>> fs/ecryptfs/crypto.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
>> index d107576..c134346 100644
>> --- a/fs/ecryptfs/crypto.c
>> +++ b/fs/ecryptfs/crypto.c
>> @@ -618,27 +618,28 @@ int ecryptfs_init_crypt_ctx(struct ecryptfs_crypt_stat *crypt_stat)
>> "key_size_bits = [%zd]\n",
>> crypt_stat->cipher, (int)strlen(crypt_stat->cipher),
>> crypt_stat->key_size << 3);
>> + mutex_lock(&crypt_stat->cs_tfm_mutex);
>> if (crypt_stat->tfm) {
>> rc = 0;
>> - goto out;
>> + goto out_unlock;
>> }
>> - mutex_lock(&crypt_stat->cs_tfm_mutex);
>> rc = ecryptfs_crypto_api_algify_cipher_name(&full_alg_name,
>> crypt_stat->cipher, "cbc");
>> if (rc)
>> goto out_unlock;
>> crypt_stat->tfm = crypto_alloc_ablkcipher(full_alg_name, 0, 0);
>> - kfree(full_alg_name);
>> if (IS_ERR(crypt_stat->tfm)) {
>> rc = PTR_ERR(crypt_stat->tfm);
>> crypt_stat->tfm = NULL;
>> ecryptfs_printk(KERN_ERR, "cryptfs: init_crypt_ctx(): "
>> "Error initializing cipher [%s]\n",
>> - crypt_stat->cipher);
>> - goto out_unlock;
>> + full_alg_name);
>> + goto out_free;
>> }
>> crypto_ablkcipher_set_flags(crypt_stat->tfm, CRYPTO_TFM_REQ_WEAK_KEY);
>> rc = 0;
>> +out_free:
>> + kfree(full_alg_name);
>> out_unlock:
>> mutex_unlock(&crypt_stat->cs_tfm_mutex);
>> out:
>
> The out label is no longer used. I removed it when I committed this
> patch.
Ah! Yes, good call. Thanks,
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-09-07 0:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-13 22:02 [PATCH] ecryptfs: avoid ctx initialization race Kees Cook
2013-09-07 0:30 ` Tyler Hicks
2013-09-07 0:46 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.