linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [bug report] evm: Check also if *tfm is an error pointer in init_desc()
@ 2020-05-12 10:48 Dan Carpenter
  2020-05-12 11:31 ` Roberto Sassu
  0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2020-05-12 10:48 UTC (permalink / raw)
  To: roberto.sassu; +Cc: linux-integrity, linux-security-module

Hello Roberto Sassu,

The patch 53de3b080d5e: "evm: Check also if *tfm is an error pointer
in init_desc()" from Apr 27, 2020, leads to the following static
checker warning:

	security/integrity/evm/evm_crypto.c:119 init_desc()
	error: '*tfm' dereferencing possible ERR_PTR()

security/integrity/evm/evm_crypto.c
    89  
    90                  tfm = &evm_tfm[hash_algo];
    91                  algo = hash_algo_name[hash_algo];
    92          }
    93  
    94          if (IS_ERR_OR_NULL(*tfm)) {

This used to be a "if (!*tfm)" check.

    95                  mutex_lock(&mutex);
    96                  if (*tfm)
    97                          goto out;

Then we test again with the lock held.  But in the new code if "*tfm"
is an error pointer then we jump directly to the unlock and crash on the
next line.  I can't see how the commit would fix anything.

    98                  *tfm = crypto_alloc_shash(algo, 0, CRYPTO_NOLOAD);
    99                  if (IS_ERR(*tfm)) {
   100                          rc = PTR_ERR(*tfm);
   101                          pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);
   102                          *tfm = NULL;
   103                          mutex_unlock(&mutex);
   104                          return ERR_PTR(rc);
   105                  }
   106                  if (type == EVM_XATTR_HMAC) {
   107                          rc = crypto_shash_setkey(*tfm, evmkey, evmkey_len);
   108                          if (rc) {
   109                                  crypto_free_shash(*tfm);
   110                                  *tfm = NULL;
   111                                  mutex_unlock(&mutex);
   112                                  return ERR_PTR(rc);
   113                          }
   114                  }
   115  out:
   116                  mutex_unlock(&mutex);
   117          }
   118  
   119          desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(*tfm),
                                                                     ^^^^
I don't understand how using *tfm outside of a lock is safe at all
anyway.

   120                          GFP_KERNEL);
   121          if (!desc)
   122                  return ERR_PTR(-ENOMEM);
   123  

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-05-14 18:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12 10:48 [bug report] evm: Check also if *tfm is an error pointer in init_desc() Dan Carpenter
2020-05-12 11:31 ` Roberto Sassu
2020-05-12 12:34   ` Dan Carpenter
2020-05-12 12:45     ` Roberto Sassu
2020-05-12 13:04       ` Dan Carpenter
2020-05-12 13:08         ` Roberto Sassu
2020-05-12 13:19         ` [PATCH] evm: Fix a small race " Dan Carpenter
2020-05-12 13:43           ` Roberto Sassu
2020-05-12 17:47             ` [PATCH v2] " Dan Carpenter
2020-05-14  6:47               ` Roberto Sassu
2020-05-14  7:11                 ` Krzysztof Struczynski
2020-05-14 18:21                   ` Mimi Zohar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).