evm: Fix a small race in init_desc()
diff mbox series

Message ID 20200512131917.GA287386@mwanda
State New
Headers show
Series
  • evm: Fix a small race in init_desc()
Related show

Commit Message

Dan Carpenter May 12, 2020, 1:19 p.m. UTC
The IS_ERR_OR_NULL() function has two conditions and if we got really
unlucky we could hit a race where "ptr" started as an error pointer and
then was set to NULL.  Both conditions would be false even though the
pointer at the end was NULL.

This patch fixes the problem by ensuring that "*tfm" can only be NULL
or valid.  I have introduced a "tmp_tfm" variable to make that work.  I
also reversed a condition and pulled the code in one tab.

Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
Fixes: 53de3b080d5e: "evm: Check also if *tfm is an error pointer in init_desc()"
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I just had a go at the fix.  I'm not super wedded to this solution, but
I hoped it was nice.

 security/integrity/evm/evm_crypto.c | 44 ++++++++++++++---------------
 1 file changed, 22 insertions(+), 22 deletions(-)

Comments

Roberto Sassu May 12, 2020, 1:43 p.m. UTC | #1
> From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
> owner@vger.kernel.org] On Behalf Of Dan Carpenter
> Sent: Tuesday, May 12, 2020 3:19 PM
> Subject: [PATCH] evm: Fix a small race in init_desc()
> 
> The IS_ERR_OR_NULL() function has two conditions and if we got really
> unlucky we could hit a race where "ptr" started as an error pointer and
> then was set to NULL.  Both conditions would be false even though the
> pointer at the end was NULL.
> 
> This patch fixes the problem by ensuring that "*tfm" can only be NULL
> or valid.  I have introduced a "tmp_tfm" variable to make that work.  I
> also reversed a condition and pulled the code in one tab.
> 
> Reported-by: Roberto Sassu <roberto.sassu@huawei.com>
> Fixes: 53de3b080d5e: "evm: Check also if *tfm is an error pointer in
> init_desc()"
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I just had a go at the fix.  I'm not super wedded to this solution, but
> I hoped it was nice.

Thanks. I think you can merge both patches in one, as the first one
is not yet pulled.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli


>  security/integrity/evm/evm_crypto.c | 44 ++++++++++++++---------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 5d3789edab71f..168c3b78ac47b 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -73,7 +73,7 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
>  {
>  	long rc;
>  	const char *algo;
> -	struct crypto_shash **tfm;
> +	struct crypto_shash **tfm, *tmp_tfm;
>  	struct shash_desc *desc;
> 
>  	if (type == EVM_XATTR_HMAC) {
> @@ -91,31 +91,31 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
>  		algo = hash_algo_name[hash_algo];
>  	}
> 
> -	if (IS_ERR_OR_NULL(*tfm)) {
> -		mutex_lock(&mutex);
> -		if (*tfm)
> -			goto out;
> -		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> -		if (IS_ERR(*tfm)) {
> -			rc = PTR_ERR(*tfm);
> -			pr_err("Can not allocate %s (reason: %ld)\n", algo,
> rc);
> -			*tfm = NULL;
> +	if (*tfm)
> +		goto alloc;
> +	mutex_lock(&mutex);
> +	if (*tfm)
> +		goto unlock;
> +
> +	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> +	if (IS_ERR(tmp_tfm)) {
> +		pr_err("Can not allocate %s (reason: %ld)\n", algo,
> +		       PTR_ERR(tmp_tfm));
> +		mutex_unlock(&mutex);
> +		return ERR_CAST(tmp_tfm);
> +	}
> +	if (type == EVM_XATTR_HMAC) {
> +		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
> +		if (rc) {
> +			crypto_free_shash(tmp_tfm);
>  			mutex_unlock(&mutex);
>  			return ERR_PTR(rc);
>  		}
> -		if (type == EVM_XATTR_HMAC) {
> -			rc = crypto_shash_setkey(*tfm, evmkey,
> evmkey_len);
> -			if (rc) {
> -				crypto_free_shash(*tfm);
> -				*tfm = NULL;
> -				mutex_unlock(&mutex);
> -				return ERR_PTR(rc);
> -			}
> -		}
> -out:
> -		mutex_unlock(&mutex);
>  	}
> -
> +	*tfm = tmp_tfm;
> +unlock:
> +	mutex_unlock(&mutex);
> +alloc:
>  	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
>  			GFP_KERNEL);
>  	if (!desc)
> --
> 2.26.2

Patch
diff mbox series

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 5d3789edab71f..168c3b78ac47b 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -73,7 +73,7 @@  static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 {
 	long rc;
 	const char *algo;
-	struct crypto_shash **tfm;
+	struct crypto_shash **tfm, *tmp_tfm;
 	struct shash_desc *desc;
 
 	if (type == EVM_XATTR_HMAC) {
@@ -91,31 +91,31 @@  static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		algo = hash_algo_name[hash_algo];
 	}
 
-	if (IS_ERR_OR_NULL(*tfm)) {
-		mutex_lock(&mutex);
-		if (*tfm)
-			goto out;
-		*tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
-		if (IS_ERR(*tfm)) {
-			rc = PTR_ERR(*tfm);
-			pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
-			*tfm = NULL;
+	if (*tfm)
+		goto alloc;
+	mutex_lock(&mutex);
+	if (*tfm)
+		goto unlock;
+
+	tmp_tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
+	if (IS_ERR(tmp_tfm)) {
+		pr_err("Can not allocate %s (reason: %ld)\n", algo,
+		       PTR_ERR(tmp_tfm));
+		mutex_unlock(&mutex);
+		return ERR_CAST(tmp_tfm);
+	}
+	if (type == EVM_XATTR_HMAC) {
+		rc = crypto_shash_setkey(tmp_tfm, evmkey, evmkey_len);
+		if (rc) {
+			crypto_free_shash(tmp_tfm);
 			mutex_unlock(&mutex);
 			return ERR_PTR(rc);
 		}
-		if (type == EVM_XATTR_HMAC) {
-			rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
-			if (rc) {
-				crypto_free_shash(*tfm);
-				*tfm = NULL;
-				mutex_unlock(&mutex);
-				return ERR_PTR(rc);
-			}
-		}
-out:
-		mutex_unlock(&mutex);
 	}
-
+	*tfm = tmp_tfm;
+unlock:
+	mutex_unlock(&mutex);
+alloc:
 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
 			GFP_KERNEL);
 	if (!desc)