| Message ID | 20200605065918.GA813@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] crc-t10dif: Fix potential crypto notify dead-lock | expand |
On Fri, Jun 05, 2020 at 04:59:18PM +1000, Herbert Xu wrote: > The crypto notify call occurs with a read mutex held so you must > not do any substantial work directly. In particular, you cannot > call crypto_alloc_* as they may trigger further notifications > which may dead-lock in the presence of another writer. > > This patch fixes this by postponing the work into a work queue and > taking the same lock in the module init function. > > While we're at it this patch also ensures that all RCU accesses are > marked appropriately (tested with sparse). > > Finally this also reveals a race condition in module param show > function as it may be called prior to the module init function. > It's fixed by testing whether crct10dif_tfm is NULL (this is true > iff the init function has not completed assuming fallback is false). > > Fixes: 11dcb1037f40 ("crc-t10dif: Allow current transform to be...") > Fixes: b76377543b73 ("crc-t10dif: Pick better transform if one...") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c > index 8cc01a603416..c9acf1c12cfc 100644 > --- a/lib/crc-t10dif.c > +++ b/lib/crc-t10dif.c > @@ -19,39 +19,46 @@ > static struct crypto_shash __rcu *crct10dif_tfm; > static struct static_key crct10dif_fallback __read_mostly; > static DEFINE_MUTEX(crc_t10dif_mutex); > +static struct work_struct crct10dif_rehash_work; > > -static int crc_t10dif_rehash(struct notifier_block *self, unsigned long val, void *data) > +static int crc_t10dif_notify(struct notifier_block *self, unsigned long val, void *data) > { > struct crypto_alg *alg = data; > - struct crypto_shash *new, *old; > > if (val != CRYPTO_MSG_ALG_LOADED || > static_key_false(&crct10dif_fallback) || > strncmp(alg->cra_name, CRC_T10DIF_STRING, strlen(CRC_T10DIF_STRING))) > return 0; > > + schedule_work(&crct10dif_rehash_work); > + return 0; > +} > + > +static void crc_t10dif_rehash(struct work_struct *work) > +{ > + struct crypto_shash *new, *old; > + > mutex_lock(&crc_t10dif_mutex); > old = rcu_dereference_protected(crct10dif_tfm, > lockdep_is_held(&crc_t10dif_mutex)); > if (!old) { > mutex_unlock(&crc_t10dif_mutex); > - return 0; > + return; > } > new = crypto_alloc_shash("crct10dif", 0, 0); > if (IS_ERR(new)) { > mutex_unlock(&crc_t10dif_mutex); > - return 0; > + return; > } > rcu_assign_pointer(crct10dif_tfm, new); > mutex_unlock(&crc_t10dif_mutex); > > synchronize_rcu(); > crypto_free_shash(old); > - return 0; > } > > static struct notifier_block crc_t10dif_nb = { > - .notifier_call = crc_t10dif_rehash, > + .notifier_call = crc_t10dif_notify, > }; > > __u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) > @@ -86,19 +93,26 @@ EXPORT_SYMBOL(crc_t10dif); > > static int __init crc_t10dif_mod_init(void) > { > + struct crypto_shash *tfm; > + > + INIT_WORK(&crct10dif_rehash_work, crc_t10dif_rehash); > crypto_register_notifier(&crc_t10dif_nb); > - crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); > - if (IS_ERR(crct10dif_tfm)) { > + mutex_lock(&crc_t10dif_mutex); > + tfm = crypto_alloc_shash("crct10dif", 0, 0); > + if (IS_ERR(tfm)) { > static_key_slow_inc(&crct10dif_fallback); > - crct10dif_tfm = NULL; > + tfm = NULL; > } > + RCU_INIT_POINTER(crct10dif_tfm, tfm); > + mutex_unlock(&crc_t10dif_mutex); > return 0; > } > > static void __exit crc_t10dif_mod_fini(void) > { > crypto_unregister_notifier(&crc_t10dif_nb); > - crypto_free_shash(crct10dif_tfm); > + cancel_work_sync(&crct10dif_rehash_work); > + crypto_free_shash(rcu_dereference_protected(crct10dif_tfm, 1)); > } > > module_init(crc_t10dif_mod_init); > @@ -106,11 +120,27 @@ module_exit(crc_t10dif_mod_fini); > > static int crc_t10dif_transform_show(char *buffer, const struct kernel_param *kp) > { > + struct crypto_shash *tfm; > + const char *name; > + int len; > + > if (static_key_false(&crct10dif_fallback)) > return sprintf(buffer, "fallback\n"); > > - return sprintf(buffer, "%s\n", > - crypto_tfm_alg_driver_name(crypto_shash_tfm(crct10dif_tfm))); > + rcu_read_lock(); > + tfm = rcu_dereference(crct10dif_tfm); > + if (!tfm) { > + len = sprintf(buffer, "init\n"); > + goto unlock; > + } Wouldn't it be better to have crct10dif_fallback enabled by default, and then disable it once the tfm is allocated? That would make the checks for a NULL tfm in crc_t10dif_transform_show() and crc_t10dif_notify() unnecessary. Also, it would make it so that crc_t10dif_update() no longer crashes if called before module_init(). - Eric
On Fri, Jun 05, 2020 at 11:22:37AM -0700, Eric Biggers wrote: > > Wouldn't it be better to have crct10dif_fallback enabled by default, and then > disable it once the tfm is allocated? That's a semantic change. As it is if the first allocation fails then we never try again. You can do this in a different patch. > That would make the checks for a NULL tfm in crc_t10dif_transform_show() and > crc_t10dif_notify() unnecessary. Also, it would make it so that > crc_t10dif_update() no longer crashes if called before module_init(). crc_t10dif_update can never be called prior to module_init while the other two functions both can. Cheers,
On Fri, Jun 05, 2020 at 11:22:37AM -0700, Eric Biggers wrote: > > Wouldn't it be better to have crct10dif_fallback enabled by default, and then > disable it once the tfm is allocated? > > That would make the checks for a NULL tfm in crc_t10dif_transform_show() and > crc_t10dif_notify() unnecessary. Also, it would make it so that > crc_t10dif_update() no longer crashes if called before module_init(). > > - Eric How about the following: (It includes some other cleanups too, like switching to the static_branch API, since the static_key one is deprecated and confusing. I can split it into separate patches...) diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index 8cc01a60341656..c45698f559cfb9 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -17,64 +17,70 @@ #include <linux/notifier.h> static struct crypto_shash __rcu *crct10dif_tfm; -static struct static_key crct10dif_fallback __read_mostly; +static DEFINE_STATIC_KEY_TRUE(crct10dif_fallback); static DEFINE_MUTEX(crc_t10dif_mutex); +static struct work_struct crct10dif_rehash_work; -static int crc_t10dif_rehash(struct notifier_block *self, unsigned long val, void *data) +static int crc_t10dif_notify(struct notifier_block *self, unsigned long val, void *data) { struct crypto_alg *alg = data; - struct crypto_shash *new, *old; if (val != CRYPTO_MSG_ALG_LOADED || - static_key_false(&crct10dif_fallback) || - strncmp(alg->cra_name, CRC_T10DIF_STRING, strlen(CRC_T10DIF_STRING))) - return 0; + strcmp(alg->cra_name, CRC_T10DIF_STRING)) + return NOTIFY_DONE; + + schedule_work(&crct10dif_rehash_work); + return NOTIFY_OK; +} + +static void crc_t10dif_rehash(struct work_struct *work) +{ + struct crypto_shash *new, *old; mutex_lock(&crc_t10dif_mutex); old = rcu_dereference_protected(crct10dif_tfm, lockdep_is_held(&crc_t10dif_mutex)); - if (!old) { - mutex_unlock(&crc_t10dif_mutex); - return 0; - } - new = crypto_alloc_shash("crct10dif", 0, 0); + new = crypto_alloc_shash(CRC_T10DIF_STRING, 0, 0); if (IS_ERR(new)) { mutex_unlock(&crc_t10dif_mutex); - return 0; + return; } rcu_assign_pointer(crct10dif_tfm, new); mutex_unlock(&crc_t10dif_mutex); - synchronize_rcu(); - crypto_free_shash(old); - return 0; + if (old) { + synchronize_rcu(); + crypto_free_shash(old); + } else { + static_branch_disable(&crct10dif_fallback); + } } static struct notifier_block crc_t10dif_nb = { - .notifier_call = crc_t10dif_rehash, + .notifier_call = crc_t10dif_notify, }; __u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) { struct { struct shash_desc shash; - char ctx[2]; + __u16 crc; } desc; int err; - if (static_key_false(&crct10dif_fallback)) + if (static_branch_unlikely(&crct10dif_fallback)) return crc_t10dif_generic(crc, buffer, len); rcu_read_lock(); desc.shash.tfm = rcu_dereference(crct10dif_tfm); - *(__u16 *)desc.ctx = crc; + desc.crc = crc; err = crypto_shash_update(&desc.shash, buffer, len); rcu_read_unlock(); BUG_ON(err); - return *(__u16 *)desc.ctx; + return desc.crc; } EXPORT_SYMBOL(crc_t10dif_update); @@ -86,19 +92,18 @@ EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void) { + INIT_WORK(&crct10dif_rehash_work, crc_t10dif_rehash); crypto_register_notifier(&crc_t10dif_nb); - crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); - if (IS_ERR(crct10dif_tfm)) { - static_key_slow_inc(&crct10dif_fallback); - crct10dif_tfm = NULL; - } + + crc_t10dif_rehash(&crct10dif_rehash_work); return 0; } static void __exit crc_t10dif_mod_fini(void) { crypto_unregister_notifier(&crc_t10dif_nb); - crypto_free_shash(crct10dif_tfm); + cancel_work_sync(&crct10dif_rehash_work); + crypto_free_shash(rcu_dereference_protected(crct10dif_tfm, 1)); } module_init(crc_t10dif_mod_init); @@ -106,14 +111,21 @@ module_exit(crc_t10dif_mod_fini); static int crc_t10dif_transform_show(char *buffer, const struct kernel_param *kp) { - if (static_key_false(&crct10dif_fallback)) + struct crypto_shash *tfm; + int len; + + if (static_branch_unlikely(&crct10dif_fallback)) return sprintf(buffer, "fallback\n"); - return sprintf(buffer, "%s\n", - crypto_tfm_alg_driver_name(crypto_shash_tfm(crct10dif_tfm))); + rcu_read_lock(); + tfm = rcu_dereference(crct10dif_tfm); + len = sprintf(buffer, "%s\n", crypto_shash_driver_name(tfm)); + rcu_read_unlock(); + + return len; } -module_param_call(transform, NULL, crc_t10dif_transform_show, NULL, 0644); +module_param_call(transform, NULL, crc_t10dif_transform_show, NULL, 0444); MODULE_DESCRIPTION("T10 DIF CRC calculation"); MODULE_LICENSE("GPL");
On Sat, Jun 06, 2020 at 04:25:22AM +1000, Herbert Xu wrote: > > That would make the checks for a NULL tfm in crc_t10dif_transform_show() and > > crc_t10dif_notify() unnecessary. Also, it would make it so that > > crc_t10dif_update() no longer crashes if called before module_init(). > > crc_t10dif_update can never be called prior to module_init while > the other two functions both can. > That's only guaranteed to be true if the code is built is a loadable module. If it's built-in to the kernel, it could be called earlier, by a previous initcall. Probably no one is running into this now, but it could happen in the future. - Eric
On Fri, Jun 05, 2020 at 11:48:46AM -0700, Eric Biggers wrote: > > That's only guaranteed to be true if the code is built is a loadable module. If > it's built-in to the kernel, it could be called earlier, by a previous initcall. That would just be a bug. The same thing can happen with any code in the kernel but we don't add NULL-pointer checks all over the place just because some buggy code could call functions before init. Cheers,
On Fri, Jun 05, 2020 at 11:45:27AM -0700, Eric Biggers wrote: > How about the following: > > (It includes some other cleanups too, like switching to the static_branch API, > since the static_key one is deprecated and confusing. I can split it into > separate patches...) I don't have objections to your patch but I think it's logically separate change and I would prefer it to go on top of my patch. Thanks,
Herbert, > The crypto notify call occurs with a read mutex held so you must not > do any substantial work directly. In particular, you cannot call > crypto_alloc_* as they may trigger further notifications which may > dead-lock in the presence of another writer. > > This patch fixes this by postponing the work into a work queue and > taking the same lock in the module init function. No particular objections to this approach from me. Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
On Fri, Jun 05, 2020 at 04:59:18PM +1000, Herbert Xu wrote: > The crypto notify call occurs with a read mutex held so you must > not do any substantial work directly. In particular, you cannot > call crypto_alloc_* as they may trigger further notifications > which may dead-lock in the presence of another writer. > > This patch fixes this by postponing the work into a work queue and > taking the same lock in the module init function. > > While we're at it this patch also ensures that all RCU accesses are > marked appropriately (tested with sparse). > > Finally this also reveals a race condition in module param show > function as it may be called prior to the module init function. > It's fixed by testing whether crct10dif_tfm is NULL (this is true > iff the init function has not completed assuming fallback is false). > > Fixes: 11dcb1037f40 ("crc-t10dif: Allow current transform to be...") > Fixes: b76377543b73 ("crc-t10dif: Pick better transform if one...") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Reviewed-by: Eric Biggers <ebiggers@google.com> - Eric
diff --git a/lib/crc-t10dif.c b/lib/crc-t10dif.c index 8cc01a603416..c9acf1c12cfc 100644 --- a/lib/crc-t10dif.c +++ b/lib/crc-t10dif.c @@ -19,39 +19,46 @@ static struct crypto_shash __rcu *crct10dif_tfm; static struct static_key crct10dif_fallback __read_mostly; static DEFINE_MUTEX(crc_t10dif_mutex); +static struct work_struct crct10dif_rehash_work; -static int crc_t10dif_rehash(struct notifier_block *self, unsigned long val, void *data) +static int crc_t10dif_notify(struct notifier_block *self, unsigned long val, void *data) { struct crypto_alg *alg = data; - struct crypto_shash *new, *old; if (val != CRYPTO_MSG_ALG_LOADED || static_key_false(&crct10dif_fallback) || strncmp(alg->cra_name, CRC_T10DIF_STRING, strlen(CRC_T10DIF_STRING))) return 0; + schedule_work(&crct10dif_rehash_work); + return 0; +} + +static void crc_t10dif_rehash(struct work_struct *work) +{ + struct crypto_shash *new, *old; + mutex_lock(&crc_t10dif_mutex); old = rcu_dereference_protected(crct10dif_tfm, lockdep_is_held(&crc_t10dif_mutex)); if (!old) { mutex_unlock(&crc_t10dif_mutex); - return 0; + return; } new = crypto_alloc_shash("crct10dif", 0, 0); if (IS_ERR(new)) { mutex_unlock(&crc_t10dif_mutex); - return 0; + return; } rcu_assign_pointer(crct10dif_tfm, new); mutex_unlock(&crc_t10dif_mutex); synchronize_rcu(); crypto_free_shash(old); - return 0; } static struct notifier_block crc_t10dif_nb = { - .notifier_call = crc_t10dif_rehash, + .notifier_call = crc_t10dif_notify, }; __u16 crc_t10dif_update(__u16 crc, const unsigned char *buffer, size_t len) @@ -86,19 +93,26 @@ EXPORT_SYMBOL(crc_t10dif); static int __init crc_t10dif_mod_init(void) { + struct crypto_shash *tfm; + + INIT_WORK(&crct10dif_rehash_work, crc_t10dif_rehash); crypto_register_notifier(&crc_t10dif_nb); - crct10dif_tfm = crypto_alloc_shash("crct10dif", 0, 0); - if (IS_ERR(crct10dif_tfm)) { + mutex_lock(&crc_t10dif_mutex); + tfm = crypto_alloc_shash("crct10dif", 0, 0); + if (IS_ERR(tfm)) { static_key_slow_inc(&crct10dif_fallback); - crct10dif_tfm = NULL; + tfm = NULL; } + RCU_INIT_POINTER(crct10dif_tfm, tfm); + mutex_unlock(&crc_t10dif_mutex); return 0; } static void __exit crc_t10dif_mod_fini(void) { crypto_unregister_notifier(&crc_t10dif_nb); - crypto_free_shash(crct10dif_tfm); + cancel_work_sync(&crct10dif_rehash_work); + crypto_free_shash(rcu_dereference_protected(crct10dif_tfm, 1)); } module_init(crc_t10dif_mod_init); @@ -106,11 +120,27 @@ module_exit(crc_t10dif_mod_fini); static int crc_t10dif_transform_show(char *buffer, const struct kernel_param *kp) { + struct crypto_shash *tfm; + const char *name; + int len; + if (static_key_false(&crct10dif_fallback)) return sprintf(buffer, "fallback\n"); - return sprintf(buffer, "%s\n", - crypto_tfm_alg_driver_name(crypto_shash_tfm(crct10dif_tfm))); + rcu_read_lock(); + tfm = rcu_dereference(crct10dif_tfm); + if (!tfm) { + len = sprintf(buffer, "init\n"); + goto unlock; + } + + name = crypto_tfm_alg_driver_name(crypto_shash_tfm(tfm)); + len = sprintf(buffer, "%s\n", name); + +unlock: + rcu_read_unlock(); + + return len; } module_param_call(transform, NULL, crc_t10dif_transform_show, NULL, 0644);
The crypto notify call occurs with a read mutex held so you must not do any substantial work directly. In particular, you cannot call crypto_alloc_* as they may trigger further notifications which may dead-lock in the presence of another writer. This patch fixes this by postponing the work into a work queue and taking the same lock in the module init function. While we're at it this patch also ensures that all RCU accesses are marked appropriately (tested with sparse). Finally this also reveals a race condition in module param show function as it may be called prior to the module init function. It's fixed by testing whether crct10dif_tfm is NULL (this is true iff the init function has not completed assuming fallback is false). Fixes: 11dcb1037f40 ("crc-t10dif: Allow current transform to be...") Fixes: b76377543b73 ("crc-t10dif: Pick better transform if one...") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>