* [PATCH 0/2] crypto: api - Fix spawn races
@ 2019-12-06 14:39 Herbert Xu
2019-12-06 14:39 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Herbert Xu @ 2019-12-06 14:39 UTC (permalink / raw)
To: Linux Crypto Mailing List
This patch series fixes a couple of race conditions in the spawn
code.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg 2019-12-06 14:39 [PATCH 0/2] crypto: api - Fix spawn races Herbert Xu @ 2019-12-06 14:39 ` Herbert Xu 2019-12-06 14:39 ` [PATCH 2/2] crypto: api - Do not zap spawn->alg Herbert Xu 2019-12-07 14:15 ` [v2 PATCH 0/2] crypto: api - Fix spawn races Herbert Xu 2 siblings, 0 replies; 11+ messages in thread From: Herbert Xu @ 2019-12-06 14:39 UTC (permalink / raw) To: Linux Crypto Mailing List The function crypto_spawn_alg is racy because it drops the lock before shooting the dying algorithm. The algorithm could disappear altogether before we shoot it. This patch fixes it by moving the shooting into the locked section. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/algapi.c | 16 +++++----------- crypto/api.c | 3 +-- crypto/internal.h | 1 - 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/crypto/algapi.c b/crypto/algapi.c index 6869feb31c99..cc55301beef4 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -678,22 +678,16 @@ EXPORT_SYMBOL_GPL(crypto_drop_spawn); static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn) { struct crypto_alg *alg; - struct crypto_alg *alg2; down_read(&crypto_alg_sem); alg = spawn->alg; - alg2 = alg; - if (alg2) - alg2 = crypto_mod_get(alg2); - up_read(&crypto_alg_sem); - - if (!alg2) { - if (alg) - crypto_shoot_alg(alg); - return ERR_PTR(-EAGAIN); + if (alg && !crypto_mod_get(alg)) { + alg->cra_flags |= CRYPTO_ALG_DYING; + alg = NULL; } + up_read(&crypto_alg_sem); - return alg; + return alg ?: ERR_PTR(-EAGAIN); } struct crypto_tfm *crypto_spawn_tfm(struct crypto_spawn *spawn, u32 type, diff --git a/crypto/api.c b/crypto/api.c index 55bca28df92d..0ef9f2a37d3d 100644 --- a/crypto/api.c +++ b/crypto/api.c @@ -344,13 +344,12 @@ static unsigned int crypto_ctxsize(struct crypto_alg *alg, u32 type, u32 mask) return len; } -void crypto_shoot_alg(struct crypto_alg *alg) +static void crypto_shoot_alg(struct crypto_alg *alg) { down_write(&crypto_alg_sem); alg->cra_flags |= CRYPTO_ALG_DYING; up_write(&crypto_alg_sem); } -EXPORT_SYMBOL_GPL(crypto_shoot_alg); struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type, u32 mask) diff --git a/crypto/internal.h b/crypto/internal.h index 93df7bec844a..e506a57e2243 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -68,7 +68,6 @@ void crypto_alg_tested(const char *name, int err); void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, struct crypto_alg *nalg); void crypto_remove_final(struct list_head *list); -void crypto_shoot_alg(struct crypto_alg *alg); struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type, u32 mask); void *crypto_create_tfm(struct crypto_alg *alg, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] crypto: api - Do not zap spawn->alg 2019-12-06 14:39 [PATCH 0/2] crypto: api - Fix spawn races Herbert Xu 2019-12-06 14:39 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu @ 2019-12-06 14:39 ` Herbert Xu 2019-12-06 22:50 ` Eric Biggers 2019-12-07 14:15 ` [v2 PATCH 0/2] crypto: api - Fix spawn races Herbert Xu 2 siblings, 1 reply; 11+ messages in thread From: Herbert Xu @ 2019-12-06 14:39 UTC (permalink / raw) To: Linux Crypto Mailing List Currently when a spawn is removed we will zap its alg field. This is racy because the spawn could belong to an unregistered instance which may dereference the spawn->alg field. This patch fixes this by keeping spawn->alg constant and instead adding a new spawn->dead field to indicate that a spawn is going away. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/algapi.c | 21 +++++++++++---------- include/crypto/algapi.h | 1 + 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/crypto/algapi.c b/crypto/algapi.c index cc55301beef4..2d0697c4f69c 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -93,15 +93,16 @@ static struct list_head *crypto_more_spawns(struct crypto_alg *alg, if (!spawn) return NULL; - n = list_next_entry(spawn, list); + list_move(&spawn->list, secondary_spawns); - if (spawn->alg && &n->list != stack && !n->alg) - n->alg = (n->list.next == stack) ? alg : - &list_next_entry(n, list)->inst->alg; + if (list_is_last(&spawn->list, stack)) + return top; - list_move(&spawn->list, secondary_spawns); + n = list_next_entry(spawn, list); + if (!spawn->dead) + n->dead = false; - return &n->list == stack ? top : &n->inst->alg.cra_users; + return &n->inst->alg.cra_users; } static void crypto_remove_instance(struct crypto_instance *inst, @@ -160,7 +161,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, if (&inst->alg == nalg) break; - spawn->alg = NULL; + spawn->dead = true; spawns = &inst->alg.cra_users; /* @@ -179,7 +180,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, &secondary_spawns))); list_for_each_entry_safe(spawn, n, &secondary_spawns, list) { - if (spawn->alg) + if (!spawn->dead) list_move(&spawn->list, &spawn->alg->cra_users); else crypto_remove_instance(spawn->inst, list); @@ -669,7 +670,7 @@ EXPORT_SYMBOL_GPL(crypto_grab_spawn); void crypto_drop_spawn(struct crypto_spawn *spawn) { down_write(&crypto_alg_sem); - if (spawn->alg) + if (!spawn->dead) list_del(&spawn->list); up_write(&crypto_alg_sem); } @@ -681,7 +682,7 @@ static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn) down_read(&crypto_alg_sem); alg = spawn->alg; - if (alg && !crypto_mod_get(alg)) { + if (!spawn->dead && !crypto_mod_get(alg)) { alg->cra_flags |= CRYPTO_ALG_DYING; alg = NULL; } diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h index 5cd846defdd6..771a295ac755 100644 --- a/include/crypto/algapi.h +++ b/include/crypto/algapi.h @@ -70,6 +70,7 @@ struct crypto_spawn { struct crypto_instance *inst; const struct crypto_type *frontend; u32 mask; + bool dead; }; struct crypto_queue { ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] crypto: api - Do not zap spawn->alg 2019-12-06 14:39 ` [PATCH 2/2] crypto: api - Do not zap spawn->alg Herbert Xu @ 2019-12-06 22:50 ` Eric Biggers 2019-12-07 3:40 ` Herbert Xu 0 siblings, 1 reply; 11+ messages in thread From: Eric Biggers @ 2019-12-06 22:50 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Fri, Dec 06, 2019 at 10:39:53PM +0800, Herbert Xu wrote: > Currently when a spawn is removed we will zap its alg field. > This is racy because the spawn could belong to an unregistered > instance which may dereference the spawn->alg field. > > This patch fixes this by keeping spawn->alg constant and instead > adding a new spawn->dead field to indicate that a spawn is going > away. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > --- > > crypto/algapi.c | 21 +++++++++++---------- > include/crypto/algapi.h | 1 + > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/crypto/algapi.c b/crypto/algapi.c > index cc55301beef4..2d0697c4f69c 100644 > --- a/crypto/algapi.c > +++ b/crypto/algapi.c > @@ -93,15 +93,16 @@ static struct list_head *crypto_more_spawns(struct crypto_alg *alg, > if (!spawn) > return NULL; > > - n = list_next_entry(spawn, list); > + list_move(&spawn->list, secondary_spawns); > > - if (spawn->alg && &n->list != stack && !n->alg) > - n->alg = (n->list.next == stack) ? alg : > - &list_next_entry(n, list)->inst->alg; > + if (list_is_last(&spawn->list, stack)) > + return top; > > - list_move(&spawn->list, secondary_spawns); > + n = list_next_entry(spawn, list); > + if (!spawn->dead) > + n->dead = false; > > - return &n->list == stack ? top : &n->inst->alg.cra_users; > + return &n->inst->alg.cra_users; > } > > static void crypto_remove_instance(struct crypto_instance *inst, > @@ -160,7 +161,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, > if (&inst->alg == nalg) > break; > > - spawn->alg = NULL; > + spawn->dead = true; > spawns = &inst->alg.cra_users; > > /* > @@ -179,7 +180,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, > &secondary_spawns))); > > list_for_each_entry_safe(spawn, n, &secondary_spawns, list) { > - if (spawn->alg) > + if (!spawn->dead) > list_move(&spawn->list, &spawn->alg->cra_users); > else > crypto_remove_instance(spawn->inst, list); > @@ -669,7 +670,7 @@ EXPORT_SYMBOL_GPL(crypto_grab_spawn); > void crypto_drop_spawn(struct crypto_spawn *spawn) > { > down_write(&crypto_alg_sem); > - if (spawn->alg) > + if (!spawn->dead) > list_del(&spawn->list); > up_write(&crypto_alg_sem); > } > @@ -681,7 +682,7 @@ static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn) > > down_read(&crypto_alg_sem); > alg = spawn->alg; > - if (alg && !crypto_mod_get(alg)) { > + if (!spawn->dead && !crypto_mod_get(alg)) { > alg->cra_flags |= CRYPTO_ALG_DYING; > alg = NULL; > } > diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h > index 5cd846defdd6..771a295ac755 100644 > --- a/include/crypto/algapi.h > +++ b/include/crypto/algapi.h > @@ -70,6 +70,7 @@ struct crypto_spawn { > struct crypto_instance *inst; > const struct crypto_type *frontend; > u32 mask; > + bool dead; > }; > This patch causes the below crash. Also, some comments (e.g. for struct crypto_spawn and crypto_remove_spawns()) would be really helpful to understand what's going on here. ================================================================== BUG: KASAN: stack-out-of-bounds in crypto_more_spawns crypto/algapi.c:105 [inline] BUG: KASAN: stack-out-of-bounds in crypto_remove_spawns+0x9cd/0xc30 crypto/algapi.c:179 Read of size 8 at addr ffff88806a1f7d58 by task cryptomgr_test/430 CPU: 2 PID: 430 Comm: cryptomgr_test Not tainted 5.4.0-07583-g2c69d0491b7e2 #5 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xa0/0xea lib/dump_stack.c:118 print_address_description.constprop.4+0x21/0x2c0 mm/kasan/report.c:374 __kasan_report.cold.8+0x7a/0xb5 mm/kasan/report.c:506 kasan_report+0x12/0x20 mm/kasan/common.c:638 __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:135 crypto_more_spawns crypto/algapi.c:105 [inline] crypto_remove_spawns+0x9cd/0xc30 crypto/algapi.c:179 crypto_alg_tested+0x493/0x5b0 crypto/algapi.c:357 cryptomgr_test+0x60/0x80 crypto/algboss.c:225 kthread+0x331/0x3f0 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 The buggy address belongs to the page: page:ffffea0001736e08 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 raw: 0100000000000000 ffffea0001736e10 ffffea0001736e10 0000000000000000 raw: 0000000000000000 0000000000000000 00000000ffffffff page dumped because: kasan: bad access detected addr ffff88806a1f7d58 is located in stack of task cryptomgr_test/430 at offset 56 in frame: crypto_remove_spawns+0x0/0xc30 crypto/algapi.c:956 this frame has 3 objects: [32, 48) 'secondary_spawns' [96, 112) 'stack' [160, 176) 'top' Memory state around the buggy address: ffff88806a1f7c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffff88806a1f7c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >ffff88806a1f7d00: 00 00 00 00 f1 f1 f1 f1 00 00 f2 f2 f2 f2 f2 f2 ^ ffff88806a1f7d80: 00 00 f2 f2 f2 f2 f2 f2 00 00 f2 f2 00 00 00 00 ffff88806a1f7e00: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 ================================================================== Disabling lock debugging due to kernel taint kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] SMP KASAN CPU: 2 PID: 430 Comm: cryptomgr_test Tainted: G B 5.4.0-07583-g2c69d0491b7e2 #5 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 RIP: 0010:crypto_remove_spawns+0x359/0xc30 crypto/algapi.c:155 Code: 39 c4 0f 84 71 01 00 00 4c 89 e0 48 c1 e8 03 42 80 3c 38 00 0f 85 a7 05 00 00 4d 8b 24 24 49 8d 7c 24 18 48 89 f8 48 c1 e8 03 <42> 80 3c 38 00 0f 85 6b 05 00 0f RSP: 0000:ffff88806a1f7cc8 EFLAGS: 00010206 RAX: 0000000000000003 RBX: ffff88806a1f7e00 RCX: ffffffff81339704 RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000018 RBP: ffff88806a1f7e28 R08: fffffbfff06973a5 R09: fffffbfff06973a5 R10: fffffbfff06973a4 R11: ffffffff834b9d23 R12: 0000000000000000 R13: ffff88806a1f7d80 R14: ffff88806a1f7d40 R15: dffffc0000000000 FS: 0000000000000000(0000) GS:ffff88806d900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000ffffffff CR3: 0000000003213001 CR4: 0000000000760ee0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 PKRU: 55555554 Call Trace: crypto_alg_tested+0x493/0x5b0 crypto/algapi.c:357 cryptomgr_test+0x60/0x80 crypto/algboss.c:225 kthread+0x331/0x3f0 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 Modules linked in: ---[ end trace e42d20303e3e7399 ]--- RIP: 0010:crypto_remove_spawns+0x359/0xc30 crypto/algapi.c:155 Code: 39 c4 0f 84 71 01 00 00 4c 89 e0 48 c1 e8 03 42 80 3c 38 00 0f 85 a7 05 00 00 4d 8b 24 24 49 8d 7c 24 18 48 89 f8 48 c1 e8 03 <42> 80 3c 38 00 0f 85 6b 05 00 0f RSP: 0000:ffff88806a1f7cc8 EFLAGS: 00010206 RAX: 0000000000000003 RBX: ffff88806a1f7e00 RCX: ffffffff81339704 RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000018 RBP: ffff88806a1f7e28 R08: fffffbfff06973a5 R09: fffffbfff06973a5 R10: fffffbfff06973a4 R11: ffffffff834b9d23 R12: 0000000000000000 R13: ffff88806a1f7d80 R14: ffff88806a1f7d40 R15: dffffc0000000000 FS: 0000000000000000(0000) GS:ffff88806d900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000ffffffff CR3: 0000000003213001 CR4: 0000000000760ee0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 PKRU: 55555554 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] crypto: api - Do not zap spawn->alg 2019-12-06 22:50 ` Eric Biggers @ 2019-12-07 3:40 ` Herbert Xu 2019-12-07 14:33 ` [PATCH] crypto: api - Add more comments to crypto_remove_spawns Herbert Xu 0 siblings, 1 reply; 11+ messages in thread From: Herbert Xu @ 2019-12-07 3:40 UTC (permalink / raw) To: Eric Biggers; +Cc: Linux Crypto Mailing List On Fri, Dec 06, 2019 at 02:50:21PM -0800, Eric Biggers wrote: > > This patch causes the below crash. Yes I got carried away with rearranging the code in the function crypto_more_spawns. I shouldn't be using spawn as a list node after doing the list_move call. The code now looks like: n = list_prev_entry(spawn, list); list_move(&spawn->list, secondary_spawns); if (list_is_last(&n->list, stack)) return top; n = list_next_entry(n, list); if (!spawn->dead) n->dead = false; return &n->inst->alg.cra_users; > Also, some comments (e.g. for struct crypto_spawn and crypto_remove_spawns()) > would be really helpful to understand what's going on here. crypto_remove_spawns is performing a depth-first walk on cra_users without recursion. In the specific case of a spawn removal triggered by a new registration, we will halt the walk when we hit the newly registered algorithm, and undo any actions that we did on the path leading to that object. The function crypto_more_spawns performs the undo action. I'll add this to crypto_remove_spawns. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] crypto: api - Add more comments to crypto_remove_spawns 2019-12-07 3:40 ` Herbert Xu @ 2019-12-07 14:33 ` Herbert Xu 0 siblings, 0 replies; 11+ messages in thread From: Herbert Xu @ 2019-12-07 14:33 UTC (permalink / raw) To: Eric Biggers; +Cc: Linux Crypto Mailing List On Sat, Dec 07, 2019 at 11:40:17AM +0800, Herbert Xu wrote: > > > Also, some comments (e.g. for struct crypto_spawn and crypto_remove_spawns()) > > would be really helpful to understand what's going on here. > > crypto_remove_spawns is performing a depth-first walk on cra_users > without recursion. In the specific case of a spawn removal triggered > by a new registration, we will halt the walk when we hit the > newly registered algorithm, and undo any actions that we did > on the path leading to that object. The function crypto_more_spawns > performs the undo action. > > I'll add this to crypto_remove_spawns. Here is the patch: ---8<--- This patch explains the logic behind crypto_remove_spawns and its underling crypto_more_spawns. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/crypto/algapi.c b/crypto/algapi.c index adb516380be9..8ef771ab0ee3 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -82,6 +82,15 @@ static void crypto_destroy_instance(struct crypto_alg *alg) crypto_tmpl_put(tmpl); } +/* + * This function adds a spawn to the list secondary_spawns which + * will be used at the end of crypto_remove_spawns to unregister + * instances, unless the spawn happens to be one that is depended + * on by the new algorithm (nalg in crypto_remove_spawns). + * + * This function is also responsible for resurrecting any algorithms + * in the dependency chain of nalg by unsetting n->dead. + */ static struct list_head *crypto_more_spawns(struct crypto_alg *alg, struct list_head *stack, struct list_head *top, @@ -128,6 +137,12 @@ static void crypto_remove_instance(struct crypto_instance *inst, BUG_ON(!list_empty(&inst->alg.cra_users)); } +/* + * Given an algorithm alg, remove all algorithms that depend on it + * through spawns. If nalg is not null, then exempt any algorithms + * that is depended on by nalg. This is useful when nalg itself + * depends on alg. + */ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, struct crypto_alg *nalg) { @@ -146,6 +161,11 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, list_move(&spawn->list, &top); } + /* + * Perform a depth-first walk starting from alg through + * the cra_users tree. The list stack records the path + * from alg to the current spawn. + */ spawns = ⊤ do { while (!list_empty(spawns)) { @@ -180,6 +200,11 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, } while ((spawns = crypto_more_spawns(alg, &stack, &top, &secondary_spawns))); + /* + * Remove all instances that are marked as dead. Also + * complete the resurrection of the others by moving them + * back to the cra_users list. + */ list_for_each_entry_safe(spawn, n, &secondary_spawns, list) { if (!spawn->dead) list_move(&spawn->list, &spawn->alg->cra_users); -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [v2 PATCH 0/2] crypto: api - Fix spawn races 2019-12-06 14:39 [PATCH 0/2] crypto: api - Fix spawn races Herbert Xu 2019-12-06 14:39 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu 2019-12-06 14:39 ` [PATCH 2/2] crypto: api - Do not zap spawn->alg Herbert Xu @ 2019-12-07 14:15 ` Herbert Xu 2019-12-07 14:15 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu 2019-12-07 14:15 ` [PATCH 2/2] crypto: api - Do not zap spawn->alg Herbert Xu 2 siblings, 2 replies; 11+ messages in thread From: Herbert Xu @ 2019-12-07 14:15 UTC (permalink / raw) To: Linux Crypto Mailing List This patch series fixes a couple of race conditions in the spawn code. v2 fixes a crash in crypto_more_spawns. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg 2019-12-07 14:15 ` [v2 PATCH 0/2] crypto: api - Fix spawn races Herbert Xu @ 2019-12-07 14:15 ` Herbert Xu 2019-12-11 3:38 ` Eric Biggers 2019-12-07 14:15 ` [PATCH 2/2] crypto: api - Do not zap spawn->alg Herbert Xu 1 sibling, 1 reply; 11+ messages in thread From: Herbert Xu @ 2019-12-07 14:15 UTC (permalink / raw) To: Linux Crypto Mailing List The function crypto_spawn_alg is racy because it drops the lock before shooting the dying algorithm. The algorithm could disappear altogether before we shoot it. This patch fixes it by moving the shooting into the locked section. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/algapi.c | 16 +++++----------- crypto/api.c | 3 +-- crypto/internal.h | 1 - 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/crypto/algapi.c b/crypto/algapi.c index 6869feb31c99..cc55301beef4 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -678,22 +678,16 @@ EXPORT_SYMBOL_GPL(crypto_drop_spawn); static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn) { struct crypto_alg *alg; - struct crypto_alg *alg2; down_read(&crypto_alg_sem); alg = spawn->alg; - alg2 = alg; - if (alg2) - alg2 = crypto_mod_get(alg2); - up_read(&crypto_alg_sem); - - if (!alg2) { - if (alg) - crypto_shoot_alg(alg); - return ERR_PTR(-EAGAIN); + if (alg && !crypto_mod_get(alg)) { + alg->cra_flags |= CRYPTO_ALG_DYING; + alg = NULL; } + up_read(&crypto_alg_sem); - return alg; + return alg ?: ERR_PTR(-EAGAIN); } struct crypto_tfm *crypto_spawn_tfm(struct crypto_spawn *spawn, u32 type, diff --git a/crypto/api.c b/crypto/api.c index 55bca28df92d..0ef9f2a37d3d 100644 --- a/crypto/api.c +++ b/crypto/api.c @@ -344,13 +344,12 @@ static unsigned int crypto_ctxsize(struct crypto_alg *alg, u32 type, u32 mask) return len; } -void crypto_shoot_alg(struct crypto_alg *alg) +static void crypto_shoot_alg(struct crypto_alg *alg) { down_write(&crypto_alg_sem); alg->cra_flags |= CRYPTO_ALG_DYING; up_write(&crypto_alg_sem); } -EXPORT_SYMBOL_GPL(crypto_shoot_alg); struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type, u32 mask) diff --git a/crypto/internal.h b/crypto/internal.h index 93df7bec844a..e506a57e2243 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -68,7 +68,6 @@ void crypto_alg_tested(const char *name, int err); void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, struct crypto_alg *nalg); void crypto_remove_final(struct list_head *list); -void crypto_shoot_alg(struct crypto_alg *alg); struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type, u32 mask); void *crypto_create_tfm(struct crypto_alg *alg, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg 2019-12-07 14:15 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu @ 2019-12-11 3:38 ` Eric Biggers 2019-12-11 5:41 ` Herbert Xu 0 siblings, 1 reply; 11+ messages in thread From: Eric Biggers @ 2019-12-11 3:38 UTC (permalink / raw) To: Herbert Xu; +Cc: Linux Crypto Mailing List On Sat, Dec 07, 2019 at 10:15:15PM +0800, Herbert Xu wrote: > The function crypto_spawn_alg is racy because it drops the lock > before shooting the dying algorithm. The algorithm could disappear > altogether before we shoot it. > > This patch fixes it by moving the shooting into the locked section. > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Does this need Fixes and Cc stable tags? - Eric ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg 2019-12-11 3:38 ` Eric Biggers @ 2019-12-11 5:41 ` Herbert Xu 0 siblings, 0 replies; 11+ messages in thread From: Herbert Xu @ 2019-12-11 5:41 UTC (permalink / raw) To: Eric Biggers; +Cc: Linux Crypto Mailing List On Tue, Dec 10, 2019 at 07:38:28PM -0800, Eric Biggers wrote: > On Sat, Dec 07, 2019 at 10:15:15PM +0800, Herbert Xu wrote: > > The function crypto_spawn_alg is racy because it drops the lock > > before shooting the dying algorithm. The algorithm could disappear > > altogether before we shoot it. > > > > This patch fixes it by moving the shooting into the locked section. > > > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > Does this need Fixes and Cc stable tags? Fixes: 6bfd48096ff8 ("[CRYPTO] api: Added spawns") I don't think we want this to go through stable right away. Perhaps a few cycles down the track it could be pushed to stable. It's certainly not an urgent problem. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] crypto: api - Do not zap spawn->alg 2019-12-07 14:15 ` [v2 PATCH 0/2] crypto: api - Fix spawn races Herbert Xu 2019-12-07 14:15 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu @ 2019-12-07 14:15 ` Herbert Xu 1 sibling, 0 replies; 11+ messages in thread From: Herbert Xu @ 2019-12-07 14:15 UTC (permalink / raw) To: Linux Crypto Mailing List Currently when a spawn is removed we will zap its alg field. This is racy because the spawn could belong to an unregistered instance which may dereference the spawn->alg field. This patch fixes this by keeping spawn->alg constant and instead adding a new spawn->dead field to indicate that a spawn is going away. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- crypto/algapi.c | 22 ++++++++++++---------- include/crypto/algapi.h | 1 + 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/crypto/algapi.c b/crypto/algapi.c index cc55301beef4..adb516380be9 100644 --- a/crypto/algapi.c +++ b/crypto/algapi.c @@ -93,15 +93,17 @@ static struct list_head *crypto_more_spawns(struct crypto_alg *alg, if (!spawn) return NULL; - n = list_next_entry(spawn, list); + n = list_prev_entry(spawn, list); + list_move(&spawn->list, secondary_spawns); - if (spawn->alg && &n->list != stack && !n->alg) - n->alg = (n->list.next == stack) ? alg : - &list_next_entry(n, list)->inst->alg; + if (list_is_last(&n->list, stack)) + return top; - list_move(&spawn->list, secondary_spawns); + n = list_next_entry(n, list); + if (!spawn->dead) + n->dead = false; - return &n->list == stack ? top : &n->inst->alg.cra_users; + return &n->inst->alg.cra_users; } static void crypto_remove_instance(struct crypto_instance *inst, @@ -160,7 +162,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, if (&inst->alg == nalg) break; - spawn->alg = NULL; + spawn->dead = true; spawns = &inst->alg.cra_users; /* @@ -179,7 +181,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list, &secondary_spawns))); list_for_each_entry_safe(spawn, n, &secondary_spawns, list) { - if (spawn->alg) + if (!spawn->dead) list_move(&spawn->list, &spawn->alg->cra_users); else crypto_remove_instance(spawn->inst, list); @@ -669,7 +671,7 @@ EXPORT_SYMBOL_GPL(crypto_grab_spawn); void crypto_drop_spawn(struct crypto_spawn *spawn) { down_write(&crypto_alg_sem); - if (spawn->alg) + if (!spawn->dead) list_del(&spawn->list); up_write(&crypto_alg_sem); } @@ -681,7 +683,7 @@ static struct crypto_alg *crypto_spawn_alg(struct crypto_spawn *spawn) down_read(&crypto_alg_sem); alg = spawn->alg; - if (alg && !crypto_mod_get(alg)) { + if (!spawn->dead && !crypto_mod_get(alg)) { alg->cra_flags |= CRYPTO_ALG_DYING; alg = NULL; } diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h index 5cd846defdd6..771a295ac755 100644 --- a/include/crypto/algapi.h +++ b/include/crypto/algapi.h @@ -70,6 +70,7 @@ struct crypto_spawn { struct crypto_instance *inst; const struct crypto_type *frontend; u32 mask; + bool dead; }; struct crypto_queue { ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-11 5:41 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-06 14:39 [PATCH 0/2] crypto: api - Fix spawn races Herbert Xu 2019-12-06 14:39 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu 2019-12-06 14:39 ` [PATCH 2/2] crypto: api - Do not zap spawn->alg Herbert Xu 2019-12-06 22:50 ` Eric Biggers 2019-12-07 3:40 ` Herbert Xu 2019-12-07 14:33 ` [PATCH] crypto: api - Add more comments to crypto_remove_spawns Herbert Xu 2019-12-07 14:15 ` [v2 PATCH 0/2] crypto: api - Fix spawn races Herbert Xu 2019-12-07 14:15 ` [PATCH 1/2] crypto: api - Fix race condition in crypto_spawn_alg Herbert Xu 2019-12-11 3:38 ` Eric Biggers 2019-12-11 5:41 ` Herbert Xu 2019-12-07 14:15 ` [PATCH 2/2] crypto: api - Do not zap spawn->alg Herbert Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.