> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Tuesday, May 12, 2020 7:47 PM
> This patch avoids a kernel panic due to accessing an error pointer set by
> crypto_alloc_shash(). It occurs especially when there are many files that
> require an unsupported algorithm, as it would increase the likelihood of
> the following race condition.
>
> Imagine we have two threads and in the first thread crypto_alloc_shash()
> fails and returns an error pointer.
>
> *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
> if (IS_ERR(*tfm)) {
> rc = PTR_ERR(*tfm); <--- FIRST THREAD HERE!
> pr_err("Can not allocate %s (reason: %ld)\n", algo,
> rc);
> *tfm = NULL;
>
> And the second thread is here:
>
> if (*tfm == NULL) { <--- SECOND THREAD HERE!
> mutex_lock(&mutex);
> if (*tfm)
> goto out;
>
> Since "*tfm" is non-NULL, we assume that it is valid and that leads to
> a crash when it dereferences "*tfm".
>
> desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
> ^^^^
>
> This patch fixes the problem by introducing a temporary "tmp_tfm" and
> only setting "*tfm" at the very end after everything has succeeded. The
> other change is that I reversed the initial "if (!*tfm) {" condition and
> pull the code in one indent level.
>
> Cc: stable at vger.kernel.org
> Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
> Reported-by: Roberto Sassu <roberto.sassu at huawei.com>
> Reported-by: Krzysztof Struczynski <krzysztof.struczynski at huawei.com>
> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
Acked-by: Roberto Sassu <roberto.sassu at huawei.com>
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> ---
> v2: I folded mine patch together with Roberto's
>
> 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 35682852ddea9..c9f7206591b30 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 (*tfm == NULL) {
> - 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