> -----Original Message----- > From: Roberto Sassu > Sent: Thursday, May 14, 2020 8:47 AM > To: Dan Carpenter <dan.carpenter@xxxxxxxxxx>; Mimi Zohar > <zohar@xxxxxxxxxxxxx>; Krzysztof Struczynski > <krzysztof.struczynski@xxxxxxxxxx> > Cc: James Morris <jmorris@xxxxxxxxx>; Serge E. Hallyn <serge@xxxxxxxxxx>; > Dmitry Kasatkin <dmitry.kasatkin@xxxxxxxxx>; linux- > integrity@xxxxxxxxxxxxxxx; linux-security-module@xxxxxxxxxxxxxxx; kernel- > janitors@xxxxxxxxxxxxxxx > Subject: RE: [PATCH v2] evm: Fix a small race in init_desc() > > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > 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@xxxxxxxxxxxxxxx > > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash") > > Reported-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > Reported-by: Krzysztof Struczynski <krzysztof.struczynski@xxxxxxxxxx> > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > Acked-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> Acked-by: Krzysztof Struczynski <krzysztof.struczynski@xxxxxxxxxx> 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
