| Message ID | 20180530202804.148245-1-mjg59@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch? If so I'll resend with a s-o-b On Wed, May 30, 2018 at 8:14 PM Wang,Junwen <wangjunwen@baidu.com> wrote: > > this pretty good, now ima_appraise & evm working well on my machine > thank you, Matthew :) > ________________________________________ > 发件人: Matthew Garrett [mjg59@google.com] > 发送时间: 2018年5月31日 4:28 > 收件人: linux-integrity@vger.kernel.org > 抄送: zohar@linux.vnet.ibm.com; Matthew Garrett; Wang,Junwen > 主题: [PATCH] evm: Don't deadlock if a crypto algorithm is unavailable > > Does this help? > > Trying to instantiate a non-existent crypto algorithm will cause the > kernel to trigger a module load. If EVM appraisal is enabled, this will > in turn trigger appraisal of the module, which will fail because the > crypto algorithm isn't available. Add a CRYPTO_NOLOAD flag and skip > module loading if it's set, and add that flag in the EVM case. > --- > crypto/api.c | 2 +- > include/linux/crypto.h | 5 +++++ > security/integrity/evm/evm_crypto.c | 3 ++- > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/crypto/api.c b/crypto/api.c > index 0ee632bba064..7aca9f86c5f3 100644 > --- a/crypto/api.c > +++ b/crypto/api.c > @@ -229,7 +229,7 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, > mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD); > > alg = crypto_alg_lookup(name, type, mask); > - if (!alg) { > + if (!alg && !(mask & CRYPTO_NOLOAD)) { > request_module("crypto-%s", name); > > if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask & > diff --git a/include/linux/crypto.h b/include/linux/crypto.h > index 6eb06101089f..e8839d3a7559 100644 > --- a/include/linux/crypto.h > +++ b/include/linux/crypto.h > @@ -112,6 +112,11 @@ > */ > #define CRYPTO_ALG_OPTIONAL_KEY 0x00004000 > > +/* > + * Don't trigger module loading > + */ > +#define CRYPTO_NOLOAD 0x00008000 > + > /* > * Transform masks and values (for crt_flags). > */ > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > index eb87d40c62a5..ff8687232a1a 100644 > --- a/security/integrity/evm/evm_crypto.c > +++ b/security/integrity/evm/evm_crypto.c > @@ -97,7 +97,8 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo) > mutex_lock(&mutex); > if (*tfm) > goto out; > - *tfm = crypto_alloc_shash(algo, 0, CRYPTO_ALG_ASYNC); > + *tfm = crypto_alloc_shash(algo, 0, > + CRYPTO_ALG_ASYNC | CRYPTO_NOLOAD); > if (IS_ERR(*tfm)) { > rc = PTR_ERR(*tfm); > pr_err("Can not allocate %s (reason: %ld)\n", algo, rc); > -- > 2.17.1.1185.g55be947832-goog >
On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote:
> Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch?
This should be posted as a separate patch, with the appropriate
"Fixes" commit info. It requires an ack from Herbert Xu.
thanks,
Mimi
On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote: > > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch? > > This should be posted as a separate patch, with the appropriate > "Fixes" commit info. It requires an ack from Herbert Xu. The non-sha1 patch doesn't seem to be in -next at the moment - should I wait for you to merge that so I can add the fixes tag?
On Thu, 2018-05-31 at 13:07 -0700, Matthew Garrett wrote: > On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote: > > > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch? > > > > This should be posted as a separate patch, with the appropriate > > "Fixes" commit info. It requires an ack from Herbert Xu. > > The non-sha1 patch doesn't seem to be in -next at the moment - should > I wait for you to merge that so I can add the fixes tag? The problem exists prior to the non-sha1 patch, but could have been resolved differently for only sha1 (eg. Kconfig requiring sha1). The non-sha1 patch requires a different solution. Junwen, is this a regression? Did a particular patch introduce this problem? Mimi
On Thu, May 31, 2018 at 1:32 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Thu, 2018-05-31 at 13:07 -0700, Matthew Garrett wrote: > > On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > > > On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote: > > > > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch? > > > > > > This should be posted as a separate patch, with the appropriate > > > "Fixes" commit info. It requires an ack from Herbert Xu. > > > > The non-sha1 patch doesn't seem to be in -next at the moment - should > > I wait for you to merge that so I can add the fixes tag? > > The problem exists prior to the non-sha1 patch, but could have been > resolved differently for only sha1 (eg. Kconfig requiring sha1). The > non-sha1 patch requires a different solution. EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok before that patch?
On Thu, 2018-05-31 at 14:06 -0700, Matthew Garrett wrote: > On Thu, May 31, 2018 at 1:32 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > On Thu, 2018-05-31 at 13:07 -0700, Matthew Garrett wrote: > > > On Thu, May 31, 2018 at 12:55 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > > > > > On Thu, 2018-05-31 at 12:30 -0700, Matthew Garrett wrote: > > > > > Cool. Mimi, does this satisfy your concerns with the non-SHA1 patch? > > > > > > > > This should be posted as a separate patch, with the appropriate > > > > "Fixes" commit info. It requires an ack from Herbert Xu. > > > > > > The non-sha1 patch doesn't seem to be in -next at the moment - should > > > I wait for you to merge that so I can add the fixes tag? > > > > The problem exists prior to the non-sha1 patch, but could have been > > resolved differently for only sha1 (eg. Kconfig requiring sha1). The > > non-sha1 patch requires a different solution. > > EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok > before that patch? According to Junwen, with CONFIG_TRUSTED_KEYS enabled the HMAC and SHA1 are allocated at __init. The locking problem occurs when CONFIG_TRUSTED_KEYS is not enabled. His solution would have been to move the crypto_alloc_shash() in EVM to an __init function. Mimi
On Fri, Jun 1, 2018 at 4:21 AM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > On Thu, 2018-05-31 at 14:06 -0700, Matthew Garrett wrote: > > EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok > > before that patch? > > According to Junwen, with CONFIG_TRUSTED_KEYS enabled the HMAC and > SHA1 are allocated at __init. The locking problem occurs when > CONFIG_TRUSTED_KEYS is not enabled. His solution would have been to > move the crypto_alloc_shash() in EVM to an __init function. Ok - I think just allowing it to be deferred is preferable, since otherwise we'd have to build in every hash algorithm that could be used for the signatures (which wasn't a problem before the non-sha1 patch). How would you prefer me to send these two? The non-sha1 patch isn't in -next, so I can't add a fixes: for it at this point.
On Fri, 2018-06-01 at 13:52 -0700, Matthew Garrett wrote: > On Fri, Jun 1, 2018 at 4:21 AM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > On Thu, 2018-05-31 at 14:06 -0700, Matthew Garrett wrote: > > > EVM looks like it SELECTs CONFIG_SHA1, so I /think/ it should be ok > > > before that patch? > > > > According to Junwen, with CONFIG_TRUSTED_KEYS enabled the HMAC and > > SHA1 are allocated at __init. The locking problem occurs when > > CONFIG_TRUSTED_KEYS is not enabled. His solution would have been to > > move the crypto_alloc_shash() in EVM to an __init function. > > Ok - I think just allowing it to be deferred is preferable, since > otherwise we'd have to build in every hash algorithm that could be > used for the signatures (which wasn't a problem before the non-sha1 > patch). How would you prefer me to send these two? The non-sha1 patch > isn't in -next, so I can't add a fixes: for it at this point. Switch the order of the two patches. The bug fix goes first, then the non-sha1 patch. Remember we need an Ack from Herbert Xu for introducing CRYPTO_NOLOAD. Mimi
diff --git a/crypto/api.c b/crypto/api.c index 0ee632bba064..7aca9f86c5f3 100644 --- a/crypto/api.c +++ b/crypto/api.c @@ -229,7 +229,7 @@ static struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, mask &= ~(CRYPTO_ALG_LARVAL | CRYPTO_ALG_DEAD); alg = crypto_alg_lookup(name, type, mask); - if (!alg) { + if (!alg && !(mask & CRYPTO_NOLOAD)) { request_module("crypto-%s", name); if (!((type ^ CRYPTO_ALG_NEED_FALLBACK) & mask & diff --git a/include/linux/crypto.h b/include/linux/crypto.h index 6eb06101089f..e8839d3a7559 100644 --- a/include/linux/crypto.h +++ b/include/linux/crypto.h @@ -112,6 +112,11 @@ */ #define CRYPTO_ALG_OPTIONAL_KEY 0x00004000 +/* + * Don't trigger module loading + */ +#define CRYPTO_NOLOAD 0x00008000 + /* * Transform masks and values (for crt_flags). */ diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index eb87d40c62a5..ff8687232a1a 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -97,7 +97,8 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo) mutex_lock(&mutex); if (*tfm) goto out; - *tfm = crypto_alloc_shash(algo, 0, CRYPTO_ALG_ASYNC); + *tfm = crypto_alloc_shash(algo, 0, + CRYPTO_ALG_ASYNC | CRYPTO_NOLOAD); if (IS_ERR(*tfm)) { rc = PTR_ERR(*tfm); pr_err("Can not allocate %s (reason: %ld)\n", algo, rc);