[2/5] evm: Check also if *tfm is an error pointer in init_desc()
diff mbox series

Message ID 20200325161116.7082-2-roberto.sassu@huawei.com
State New
Headers show
Series
  • [1/5] ima: Set file->f_mode instead of file->f_flags in ima_calc_file_hash()
Related show

Commit Message

Roberto Sassu March 25, 2020, 4:11 p.m. UTC
The mutex in init_desc(), introduced by commit 97426f985729 ("evm: prevent
racing during tfm allocation") prevents two tasks to concurrently set *tfm.
However, checking if *tfm is NULL is not enough, as crypto_alloc_shash()
can return an error pointer. The following sequence can happen:

Task A: *tfm = crypto_alloc_shash() <= error pointer
Task B: if (*tfm == NULL) <= *tfm is not NULL, use it
Task B: rc = crypto_shash_init(desc) <= panic
Task A: *tfm = NULL

This patch uses the IS_ERR_OR_NULL macro to determine whether or not a new
crypto context must be created.

Cc: stable@vger.kernel.org
Fixes: 97426f985729 ("evm: prevent racing during tfm allocation")
Co-developed-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mimi Zohar April 22, 2020, 1:45 p.m. UTC | #1
Hi Roberto, Krzysztof,

On Wed, 2020-03-25 at 17:11 +0100, Roberto Sassu wrote:
> The mutex in init_desc(), introduced by commit 97426f985729 ("evm: prevent
> racing during tfm allocation") prevents two tasks to concurrently set *tfm.
> However, checking if *tfm is NULL is not enough, as crypto_alloc_shash()
> can return an error pointer. The following sequence can happen:
> 
> Task A: *tfm = crypto_alloc_shash() <= error pointer
> Task B: if (*tfm == NULL) <= *tfm is not NULL, use it
> Task B: rc = crypto_shash_init(desc) <= panic
> Task A: *tfm = NULL
> 
> This patch uses the IS_ERR_OR_NULL macro to determine whether or not a new
> crypto context must be created.
> 
> Cc: stable@vger.kernel.org
> Fixes: 97426f985729 ("evm: prevent racing during tfm allocation")

Thank you.  True, this commit introduced the mutex, but the actual
problem is most likely the result of a crypto algorithm not being
configured.  Depending on the kernel and which crypto algorithms are
enabled, verifying an EVM signature might not be possible.  In the
embedded environment, where the entire filesystem is updated, there
shouldn't be any unknown EVM signature algorithms.

In case Greg or Sasha decide this patch should be backported,
including the context/motivation in the patch description (first
paragraph) would be helpful.

Mimi

> Co-developed-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_crypto.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index 35682852ddea..77ad1e5a93e4 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -91,7 +91,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>  		algo = hash_algo_name[hash_algo];
>  	}
>  
> -	if (*tfm == NULL) {
> +	if (IS_ERR_OR_NULL(*tfm)) {
>  		mutex_lock(&mutex);
>  		if (*tfm)
>  			goto out;
Roberto Sassu April 22, 2020, 3:37 p.m. UTC | #2
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Wednesday, April 22, 2020 3:45 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Krzysztof Struczynski
> <krzysztof.struczynski@huawei.com>; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH 2/5] evm: Check also if *tfm is an error pointer in
> init_desc()
> 
> Hi Roberto, Krzysztof,
> 
> On Wed, 2020-03-25 at 17:11 +0100, Roberto Sassu wrote:
> > The mutex in init_desc(), introduced by commit 97426f985729 ("evm:
> prevent
> > racing during tfm allocation") prevents two tasks to concurrently set *tfm.
> > However, checking if *tfm is NULL is not enough, as crypto_alloc_shash()
> > can return an error pointer. The following sequence can happen:
> >
> > Task A: *tfm = crypto_alloc_shash() <= error pointer
> > Task B: if (*tfm == NULL) <= *tfm is not NULL, use it
> > Task B: rc = crypto_shash_init(desc) <= panic
> > Task A: *tfm = NULL
> >
> > This patch uses the IS_ERR_OR_NULL macro to determine whether or not
> a new
> > crypto context must be created.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 97426f985729 ("evm: prevent racing during tfm allocation")
> 
> Thank you.  True, this commit introduced the mutex, but the actual
> problem is most likely the result of a crypto algorithm not being
> configured.  Depending on the kernel and which crypto algorithms are
> enabled, verifying an EVM signature might not be possible.  In the
> embedded environment, where the entire filesystem is updated, there
> shouldn't be any unknown EVM signature algorithms.

Hi Mimi

right, the actual commit that introduced the issue is:

d46eb3699502b ("evm: crypto hash replaced by shash")

> In case Greg or Sasha decide this patch should be backported,
> including the context/motivation in the patch description (first
> paragraph) would be helpful.

Ok. The main motivation is to avoid kernel panic, especially if there
are many files that require an unsupported hash algorithm, as it would
increase the likelihood of the race condition I described.

Roberto

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


> > Co-developed-by: Krzysztof Struczynski
> <krzysztof.struczynski@huawei.com>
> > Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_crypto.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> > index 35682852ddea..77ad1e5a93e4 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -91,7 +91,7 @@ static struct shash_desc *init_desc(char type, uint8_t
> hash_algo)
> >  		algo = hash_algo_name[hash_algo];
> >  	}
> >
> > -	if (*tfm == NULL) {
> > +	if (IS_ERR_OR_NULL(*tfm)) {
> >  		mutex_lock(&mutex);
> >  		if (*tfm)
> >  			goto out;

Patch
diff mbox series

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 35682852ddea..77ad1e5a93e4 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -91,7 +91,7 @@  static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		algo = hash_algo_name[hash_algo];
 	}
 
-	if (*tfm == NULL) {
+	if (IS_ERR_OR_NULL(*tfm)) {
 		mutex_lock(&mutex);
 		if (*tfm)
 			goto out;