* [PATCH] evm: Fix a small race in init_desc() [not found] <20200512130410.GB2056@kadam> @ 2020-05-12 13:19 ` Dan Carpenter 2020-05-12 13:43 ` Roberto Sassu 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2020-05-12 13:19 UTC (permalink / raw) To: Mimi Zohar, Roberto Sassu Cc: James Morris, Serge E. Hallyn, linux-integrity, linux-security-module, kernel-janitors The IS_ERR_OR_NULL() function has two conditions and if we got really unlucky we could hit a race where "ptr" started as an error pointer and then was set to NULL. Both conditions would be false even though the pointer at the end was NULL. This patch fixes the problem by ensuring that "*tfm" can only be NULL or valid. I have introduced a "tmp_tfm" variable to make that work. I also reversed a condition and pulled the code in one tab. Reported-by: Roberto Sassu <roberto.sassu@huawei.com> Fixes: 53de3b080d5e: "evm: Check also if *tfm is an error pointer in init_desc()" Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- I just had a go at the fix. I'm not super wedded to this solution, but I hoped it was nice. 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 5d3789edab71f..168c3b78ac47b 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 (IS_ERR_OR_NULL(*tfm)) { - 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] evm: Fix a small race in init_desc() 2020-05-12 13:19 ` [PATCH] evm: Fix a small race in init_desc() Dan Carpenter @ 2020-05-12 13:43 ` Roberto Sassu 2020-05-12 17:47 ` [PATCH v2] " Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Roberto Sassu @ 2020-05-12 13:43 UTC (permalink / raw) To: Dan Carpenter, Mimi Zohar Cc: James Morris, Serge E. Hallyn, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-janitors@vger.kernel.org, Silviu Vlasceanu > From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity- > owner@vger.kernel.org] On Behalf Of Dan Carpenter > Sent: Tuesday, May 12, 2020 3:19 PM > Subject: [PATCH] evm: Fix a small race in init_desc() > > The IS_ERR_OR_NULL() function has two conditions and if we got really > unlucky we could hit a race where "ptr" started as an error pointer and > then was set to NULL. Both conditions would be false even though the > pointer at the end was NULL. > > This patch fixes the problem by ensuring that "*tfm" can only be NULL > or valid. I have introduced a "tmp_tfm" variable to make that work. I > also reversed a condition and pulled the code in one tab. > > Reported-by: Roberto Sassu <roberto.sassu@huawei.com> > Fixes: 53de3b080d5e: "evm: Check also if *tfm is an error pointer in > init_desc()" > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > I just had a go at the fix. I'm not super wedded to this solution, but > I hoped it was nice. Thanks. I think you can merge both patches in one, as the first one is not yet pulled. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > 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 5d3789edab71f..168c3b78ac47b 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 (IS_ERR_OR_NULL(*tfm)) { > - 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] evm: Fix a small race in init_desc() 2020-05-12 13:43 ` Roberto Sassu @ 2020-05-12 17:47 ` Dan Carpenter 2020-05-14 6:47 ` Roberto Sassu 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2020-05-12 17:47 UTC (permalink / raw) To: Mimi Zohar, Roberto Sassu, Krzysztof Struczynski Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity, linux-security-module, kernel-janitors 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@vger.kernel.org Fixes: d46eb3699502b ("evm: crypto hash replaced by shash") Reported-by: Roberto Sassu <roberto.sassu@huawei.com> Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH v2] evm: Fix a small race in init_desc() 2020-05-12 17:47 ` [PATCH v2] " Dan Carpenter @ 2020-05-14 6:47 ` Roberto Sassu 2020-05-14 7:11 ` Krzysztof Struczynski 0 siblings, 1 reply; 6+ messages in thread From: Roberto Sassu @ 2020-05-14 6:47 UTC (permalink / raw) To: Dan Carpenter, Mimi Zohar, Krzysztof Struczynski Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-janitors@vger.kernel.org > From: Dan Carpenter [mailto:dan.carpenter@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@vger.kernel.org > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash") > Reported-by: Roberto Sassu <roberto.sassu@huawei.com> > Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Acked-by: Roberto Sassu <roberto.sassu@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2] evm: Fix a small race in init_desc() 2020-05-14 6:47 ` Roberto Sassu @ 2020-05-14 7:11 ` Krzysztof Struczynski 2020-05-14 18:21 ` Mimi Zohar 0 siblings, 1 reply; 6+ messages in thread From: Krzysztof Struczynski @ 2020-05-14 7:11 UTC (permalink / raw) To: Roberto Sassu, Dan Carpenter, Mimi Zohar Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-janitors@vger.kernel.org > -----Original Message----- > From: Roberto Sassu > Sent: Thursday, May 14, 2020 8:47 AM > To: Dan Carpenter <dan.carpenter@oracle.com>; Mimi Zohar > <zohar@linux.ibm.com>; Krzysztof Struczynski > <krzysztof.struczynski@huawei.com> > Cc: James Morris <jmorris@namei.org>; Serge E. Hallyn <serge@hallyn.com>; > Dmitry Kasatkin <dmitry.kasatkin@nokia.com>; linux- > integrity@vger.kernel.org; linux-security-module@vger.kernel.org; kernel- > janitors@vger.kernel.org > Subject: RE: [PATCH v2] evm: Fix a small race in init_desc() > > > From: Dan Carpenter [mailto:dan.carpenter@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@vger.kernel.org > > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash") > > Reported-by: Roberto Sassu <roberto.sassu@huawei.com> > > Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Acked-by: Roberto Sassu <roberto.sassu@huawei.com> Acked-by: Krzysztof Struczynski <krzysztof.struczynski@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] evm: Fix a small race in init_desc() 2020-05-14 7:11 ` Krzysztof Struczynski @ 2020-05-14 18:21 ` Mimi Zohar 0 siblings, 0 replies; 6+ messages in thread From: Mimi Zohar @ 2020-05-14 18:21 UTC (permalink / raw) To: Krzysztof Struczynski, Roberto Sassu, Dan Carpenter Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-janitors@vger.kernel.org On Thu, 2020-05-14 at 07:11 +0000, Krzysztof Struczynski wrote: > > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > > > 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@vger.kernel.org > > > Fixes: d46eb3699502b ("evm: crypto hash replaced by shash") > > > Reported-by: Roberto Sassu <roberto.sassu@huawei.com> > > > Reported-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Acked-by: Roberto Sassu <roberto.sassu@huawei.com> > > Acked-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com> Thanks, Roberto and Krzysztof. This patch is now queued in the "fixes" branch. Mimi ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-14 18:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200512130410.GB2056@kadam>
2020-05-12 13:19 ` [PATCH] evm: Fix a small race in init_desc() 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).