[PATCH v2] evm: Fix a small race in init_desc()

[Krzysztof Struczynski]

> -----Original Message-----
> From: Roberto Sassu
> Sent: Thursday, May 14, 2020 8:47 AM
> To: Dan Carpenter <dan.carpenter at oracle.com>; Mimi Zohar
> <zohar at linux.ibm.com>; Krzysztof Struczynski
> <krzysztof.struczynski at huawei.com>
> Cc: James Morris <jmorris at namei.org>; Serge E. Hallyn <serge at hallyn.com>;
> Dmitry Kasatkin <dmitry.kasatkin at nokia.com>; linux-
> integrity at vger.kernel.org; linux-security-module at vger.kernel.org; kernel-
> janitors at vger.kernel.org
> Subject: RE: [PATCH v2] evm: Fix a small race in init_desc()
> 
> > 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>

Acked-by: Krzysztof Struczynski <krzysztof.struczynski at huawei.com>

Krzysztof
> 
> 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