linux-kernel - Re: [PATCH 2/5] evm: Check also if *tfm is an error pointer in init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1587563102.5738.32.camel@linux.ibm.com>
Date:   Wed, 22 Apr 2020 09:45:02 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Roberto Sassu <roberto.sassu@...wei.com>
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, krzysztof.struczynski@...wei.com,
        silviu.vlasceanu@...wei.com, stable@...r.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@...r.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@...wei.com>
> Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@...wei.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.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;